New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: agent config plugin serialising only public fields #249
Conversation
varas
commented
Nov 24, 2020
•
edited
edited
- agent-config plugin serialising now avoids non public config fields.
- agent-config plugin now submit inventory items in config format, ie. snake-case as YAML (instead of previous camel-case variable name).
b70d2c8
to
306b88a
Compare
Pull Request Test Coverage Report for Build 487640757
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
pkg/plugins/agent_config.go
Outdated
continue | ||
} | ||
firstLetter := rune(name[0]) | ||
if unicode.IsLower(firstLetter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we filter them by the "public" annotation?
https://github.com/newrelic/infrastructure-agent/blob/master/pkg/config/config.go#L64
In case that a config option needs to be public in the code but we don't wan't to send the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that "public" annotation belongs to the past agent closed source history, where we wanted to know whether a config option was meant to be publicly announced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW do we still use this "public" anno for something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used for discriminating which fields should be logged and now we'll be using it for agent config inventory plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one small question
https://github.com/newrelic/infrastructure-agent/pull/249/files#r530889786
83797d1
ac3de28
to
83797d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
83797d1
to
90f732f
Compare
90f732f
to
f90383f
Compare