Skip to content
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: overlay IPAM not reporting version #2090

Merged
merged 7 commits into from
Aug 14, 2023
Merged

fix: overlay IPAM not reporting version #2090

merged 7 commits into from
Aug 14, 2023

Conversation

nddq
Copy link
Contributor

@nddq nddq commented Jul 27, 2023

Reason for Change:

Issue Fixed:

Fixes #2081

Requirements:

Notes:

@nddq nddq requested a review from a team as a code owner July 27, 2023 22:45
@nddq nddq requested a review from ashvindeodhar July 27, 2023 22:45
@rbtr
Copy link
Contributor

rbtr commented Jul 31, 2023

@nddq what's the output of ./azure-ipam -v with this change?

@@ -1,6 +1,6 @@
package buildinfo
package buildversion
Copy link
Contributor

Choose a reason for hiding this comment

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

why renaming and moving these around?

Copy link
Contributor

Choose a reason for hiding this comment

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

this file should be deleted entirely

Makefile Outdated
@@ -173,7 +173,7 @@ zapai-version: ## prints the zapai version

# Build the delegated IPAM plugin binary.
azure-ipam-binary:
cd $(AZURE_IPAM_DIR) && CGO_ENABLED=0 go build -v -o $(AZURE_IPAM_BUILD_DIR)/azure-ipam$(EXE_EXT) -ldflags "-X main.version=$(AZURE_IPAM_VERSION)" -gcflags="-dwarflocationlists=true"
cd $(AZURE_IPAM_DIR) && CGO_ENABLED=0 go build -v -o $(AZURE_IPAM_BUILD_DIR)/azure-ipam$(EXE_EXT) -ldflags "-X buildversion.BuildVersion=$(AZURE_IPAM_VERSION)" -gcflags="-dwarflocationlists=true"
Copy link
Contributor

Choose a reason for hiding this comment

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

could just change this to buildinfo.Version without renaming/moving the files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to match and see if this work but no luck. I did tried populating buildversion.BuildVersion directly before but no luck either. The only way I managed to get the version printed out is to have the version variable inside main then populate it through main.Version like CNI and CNS does right now. Is this the only way or I'm I missing something?

Copy link
Contributor

@rbtr rbtr Jul 31, 2023

Choose a reason for hiding this comment

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

what you're missing is that main.version works in our linker args because main is the top level package and we have the version var defined there. to inject a value more deeply, we need to fully-qualify the path.

read the comments in the file you linked and check the mentioned "linker script", you'll see how we need to pass the full path to the buildversion package and variable for the injection to work. we should not need to change anything except the "-X [...]=$(AZURE_IPAM_VERSION)". our Makefile will automatically set that AZURE_IPAM_VERSION env var at runtime to either the bare tag if the current revision is tagged, or the latest tag + dirty hash if the current rev is not tagged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, working now

@nddq
Copy link
Contributor Author

nddq commented Jul 31, 2023

@nddq what's the output of ./azure-ipam -v with this change?

image
This is what I got currently. Should azure-ipam have a release tag like v1.xxx+commit like CNI and CNS or is this intended? I remember that we discussed about wanting to tag and release each plugin individually last summer so it would be great if we could follow up on this was well.

@nddq nddq requested a review from rbtr August 8, 2023 20:02
@nddq nddq requested a review from thatmattlong August 9, 2023 16:40
@rbtr rbtr requested a review from matmerr August 9, 2023 17:57
@rbtr rbtr assigned nddq Aug 9, 2023
@rbtr rbtr added the fix Fixes something. label Aug 9, 2023
@rbtr rbtr merged commit 19e42c4 into Azure:master Aug 14, 2023
rbtr pushed a commit that referenced this pull request Sep 8, 2023
* fix overlay IPAM not reporting version

* revert file and var naming, add correct path to makefile
jpayne3506 pushed a commit that referenced this pull request Sep 11, 2023
* fix overlay IPAM not reporting version

* revert file and var naming, add correct path to makefile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
azure-ipam fix Fixes something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

overlay IPAM not reporting version
5 participants