Add a User-Agent header to metadata API requests; allow overriding via config; add --version argument.#84
Conversation
supriyagarg
left a comment
supriyagarg
left a comment
There was a problem hiding this comment.
minor comments / questions.
| PKG_NAME=stackdriver-metadata | ||
| PKG_VERSION=0.0.17 | ||
| PKG_RELEASE=1 | ||
| PKG_MAINTAINER=Stackdriver Agents <stackdriver-agents@google.com> |
There was a problem hiding this comment.
How about moving all variables starting with PKG_ and DOCKER_ to the top so that they are defined in the same place?
There was a problem hiding this comment.
I was debating whether to do this. At this point, only versions (that will be changed with each subsequent release) live at the top. Adding more stuff there may be confusing to readers.
There was a problem hiding this comment.
Okay, that makes sense.
| #endif | ||
|
|
||
| #define STRINGIFY_H(x) #x | ||
| #define STRINGIFY(x) STRINGIFY_H(x) |
There was a problem hiding this comment.
why are two macro definitions required? Isn't the first one sufficient?
There was a problem hiding this comment.
Macros are substituted outside-in. If we simply called STRINGIFY_H(AGENT_VERSION), the result would have been the string "AGENT_VERSION" (because #x would be evaluated with x set to AGENT_VERSION). Adding another macro layer will expand both macros in the first pass, so STRINGIFY(AGENT_VERSION) expands to STRINGIFY_H(0.0.17-1) (assuming AGENT_VERSION is set to 0.0.17-1), which then expands on the next pass to #x with x set to 0.0.17-1, resulting in the string "0.0.17-1". The C preprocessor is weird.
There was a problem hiding this comment.
thanks for the explanation. Please add a unit test for it to make this more explicit.
There was a problem hiding this comment.
I actually tried. Doing this right would require rebuilding configuration.cc with a different AGENT_VERSION value (changing the way we build files under test), which is more upheaval than this PR warrants.
I suppose I could pull the STRINGIFY macro out into one of the headers (format.h?) and test it separately... Let me know if you want me to do that.
There was a problem hiding this comment.
As discussed offline, referenced the GCC docs instead.
| PKG_NAME=stackdriver-metadata | ||
| PKG_VERSION=0.0.17 | ||
| PKG_RELEASE=1 | ||
| PKG_MAINTAINER=Stackdriver Agents <stackdriver-agents@google.com> |
There was a problem hiding this comment.
I was debating whether to do this. At this point, only versions (that will be changed with each subsequent release) live at the top. Adding more stuff there may be confusing to readers.
| #endif | ||
|
|
||
| #define STRINGIFY_H(x) #x | ||
| #define STRINGIFY(x) STRINGIFY_H(x) |
There was a problem hiding this comment.
Macros are substituted outside-in. If we simply called STRINGIFY_H(AGENT_VERSION), the result would have been the string "AGENT_VERSION" (because #x would be evaluated with x set to AGENT_VERSION). Adding another macro layer will expand both macros in the first pass, so STRINGIFY(AGENT_VERSION) expands to STRINGIFY_H(0.0.17-1) (assuming AGENT_VERSION is set to 0.0.17-1), which then expands on the next pass to #x with x set to 0.0.17-1, resulting in the string "0.0.17-1". The C preprocessor is weird.
|
|
||
| if (flags.count("help")) { | ||
| std::cout << flags_desc << std::endl; | ||
| return 0; |
There was a problem hiding this comment.
Previously help returned 1, was this change intended?
There was a problem hiding this comment.
Ah, yes, it was. It's a bug fix. There's nothing wrong with asking the program to print a help message, so it shouldn't return a failure exit code.
There was a problem hiding this comment.
Turns out this bug fix introduced another bug -- we were using the nonzero return value of ParseArguments to cause the program to exit. Just fixed in another commit, which also added handling of argument parse errors and printing the version correctly.
…ents return value bug.
|
|
||
| if (flags.count("help")) { | ||
| std::cout << flags_desc << std::endl; | ||
| return 0; |
There was a problem hiding this comment.
Turns out this bug fix introduced another bug -- we were using the nonzero return value of ParseArguments to cause the program to exit. Just fixed in another commit, which also added handling of argument parse errors and printing the version correctly.
igorpeshansky
changed the title
4 participants
@Georgehe4 FYI.