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

show vsphere-cloud-controller-manager version in the log correctly #535

Merged
merged 1 commit into from
Dec 4, 2021

Conversation

nicolehanjing
Copy link
Member

What this PR does / why we need it:
vsphere-cloud-controller-manager version is shown as ${VERSION} in the log:

I1201 10:00:32.996286       1 main.go:81] vsphere-cloud-controller-manager version: ${VERSION}

we should see a version number instead of ${VERSION}.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #533

Special notes for your reviewer:
verified the fix:

I1203 00:09:19.942725       1 main.go:81] vsphere-cloud-controller-manager version: 1.22.3

Release note:

show vsphere-cloud-controller-manager version in the log correctly

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 3, 2021
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021
@akutz
Copy link
Member

akutz commented Dec 3, 2021

I think this fix is a good start, but shouldn't the fix be passing the version into the Docker build command as a build arg instead of hard coding it?

@nicolehanjing
Copy link
Member Author

I think this fix is a good start, but shouldn't the fix be passing the version into the Docker build command as a build arg instead of hard coding it?

I think either works - using the fixed value and updating it when we have a new version released; or fetch the version value via git commands
I think using a fixed value can help to prevent possible issues in some other environments, and I'm planning to add this in my WIP release doc to inform the maintainer to update the version value when a new release version is available.

@@ -327,4 +327,5 @@ squash:
docker-image:
docker build \
-f cluster/images/controller-manager/Dockerfile \
-t "$(IMAGE):$(BRANCH_NAME)" . \
-t "$(IMAGE):$(BRANCH_NAME)" \
--build-arg "VERSION=${VERSION}" . \
Copy link
Member Author

Choose a reason for hiding this comment

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

pass version info dynamically when building docker image

@@ -33,7 +33,7 @@ ARG DISTROLESS_IMAGE=gcr.io/distroless/static@sha256:9b60270ec0991bc4f14bda475e8
FROM ${GOLANG_IMAGE} as builder

# This build arg is the version to embed in the CPI binary
ARG VERSION=unknown
ARG VERSION=1.22.3
Copy link
Member Author

Choose a reason for hiding this comment

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

for local development, if you run docker build command without version info, the log is still not correct. Probably we can leave the fixed value and update it when we have a new release version

@lubronzhan
Copy link
Contributor

/lgtm
/approve
/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Dec 4, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lubronzhan, nicolehanjing

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [lubronzhan,nicolehanjing]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit a5c6113 into master Dec 4, 2021
@lubronzhan lubronzhan deleted the nicoleh-fix-format branch October 19, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vsphere-cloud-controller-manager version is not shown in the log correctly
4 participants