fix(understackctl): make version stamping actually work - #2192
Conversation
|
LGTM by looking at the code. I haven't tried the ctl manually myself on my laptop. Is there a definitive guide which I could follow to test this myself? Would I require a dev setup of understack in my local to test this? |
https://rackerlabs.github.io/understack/operator-guide/understackctl/ |
The Makefile has always built LDFLAGS with -X main.version and -X main.commit, but neither variable existed and nothing exposed a --version flag. build-all also expanded $() instead of $(LDFLAGS). Declare both variables, wire them into `understackctl --version`.
ca6bbb3 to
d6effa3
Compare
|
I thought we decided to sepearate version change and the change in the MAKEFILE separate. |
One doesn't work without the other. There's no way to test them. You need to have LDFLAGS passed along to get the version info passed in. |
|
I get it now, I had a gap in my knowledge here. It is clear to me now. I faintly remember that LDFLAGS were used in other projects, but you have defined it earlier in MAKEFILE which has the keyword version in it. Hence the confusion. Since above line was earlier added at some other epoch of time. I could not relate to it. |
|
I'm not sure what you're looking for me to change code wise. Will you just make the change? |
|
It looks good to me. No change is required. |
Okay. Will you approve the PR then? |
|
Done! |
2 participants
The Makefile has always built LDFLAGS with -X main.version and$() instead of $ (LDFLAGS).
-X main.commit, but neither variable existed and nothing exposed a
--version flag. build-all also expanded
Declare both variables, wire them into
understackctl --version.