Skip to content

Conversation

@johvet
Copy link

@johvet johvet commented Aug 12, 2020

This PR addresses an issue when trying to configure a collector with a repeated (portion) of the name, for example Jolokia. This would match CassandraJolokiaCollector.conf, JolokiaCollector.conf, and KafkaJolokiaCollector.conf and the entrypoint.sh fails. Same applies to Http.

@rclagett rclagett changed the base branch from master to dev August 12, 2020 20:12
@rclagett
Copy link
Contributor

LGTM thanks for contributing @johvet !

Going to have the original author @nhatfield take a look as well.

if [[ "${v}" == "COLLECTOR_"* ]]; then

FILE=`echo "${COLLECTORS}" | grep -i "$(echo "${v}" | sed 's%^COLLECTOR_%%;s%_.*%%' | tr 'A-Z' 'a-z')"`
FILE=`echo "${COLLECTORS}" | grep -i "^$(echo "${v}" | sed 's%^COLLECTOR_%%;s%_.*%%' | tr 'A-Z' 'a-z')$"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johvet I agree this needs to be done. Would you mind if we do this native in grep? Just adding -w before or after the -i will do the trick. Appreciate your contribution

Thanks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, using -w is way more elegant. Wasn't thinking about that.

@TheConnMan
Copy link
Contributor

@johvet: @rclagett rebased your PR onto dev and there's some commits from master we'd like to remove from your branch. Could you run the following to cherry-pick your commit onto dev and then force push to this PR?

git checkout johvet/support-repeated-collector-names
git status // make sure the working directory is clean
git reset --hard dev // hard reset your branch to the dev branch
git cherry-pick 1aedda7 // cherry pick your commit onto dev
git push -f // force push to this PR

@nhatfield nhatfield self-requested a review August 14, 2020 13:16
@rclagett
Copy link
Contributor

Hi @johvet are you okay to make the changes @nhatfield and @TheConnMan suggested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants