-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
scaffold: printVersion includes operator's Version #1953
scaffold: printVersion includes operator's Version #1953
Conversation
Hi @drnic. Thanks for your PR. I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@drnic thanks for submitting a PR! This change is a nice-to-have. /ok-to-test |
b562ed8
to
4dbafc4
Compare
I’ll investigate the CI failure soon. Sorry about that.
|
4dbafc4
to
9630552
Compare
Any pro tips for debugging/fixing this?
|
/test e2e-aws-go |
Hurray! The simple delights of CI passing. |
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.
Hi @djzager,
Thank you for your contribution.
It is missing the CHANGELOG entry. Could you please add it?
Also, IMHO it would be better Operator Version
instead of just Version
in order to make clear from where came the version. Do not you agree? WDYT?
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.
It was tested locally and all worked fine.
Asked for a few nits and add Changelog.
/lgtm
/approve
$ operator-sdk up local
INFO[0000] Running the operator locally.
INFO[0000] Using namespace default.
{"level":"info","ts":1570560195.7755609,"logger":"cmd","msg":"Version: 0.0.1"}
{"level":"info","ts":1570560195.775807,"logger":"cmd","msg":"Go Version: go1.12.10"}
{"level":"info","ts":1570560195.775821,"logger":"cmd","msg":"Go OS/Arch: darwin/amd64"}
{"level":"info","ts":1570560195.775827,"logger":"cmd","msg":"Version of operator-sdk: v0.10.0+git"}
{"level":"info","ts":1570560195.776957,"logger":"leader","msg":"Trying to become the leader."}
{"level":"info","ts":1570560195.7769878,"logger":"leader","msg":"Skipping leader election; not running in a cluster."}
9630552
to
0ed388e
Compare
Fixed minor suggestions + rebased against master. |
/lgtm |
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 once CHANGELOG is updated.
Hi @drnic, It still missing the changelog entry. The commit for it maybe needs to be pushed. |
0ed388e
to
396fdbe
Compare
New changes are detected. LGTM label has been removed. |
Sorry, I missed the request for CHANGELOG update earlier. |
CHANGELOG.md
Outdated
@@ -8,6 +8,7 @@ | |||
- Ansible based operators now gather and serve metrics about each custom resource on port 8686 of the metrics service. ([#1723](https://github.com/operator-framework/operator-sdk/pull/1723)) | |||
- Added the Go version, OS, and architecture to the output of `operator-sdk version` ([#1863](https://github.com/operator-framework/operator-sdk/pull/1863)) | |||
- Added support for `ppc64le-linux` for the `operator-sdk` binary and the Helm operator base image. ([#1533](https://github.com/operator-framework/operator-sdk/pull/1533)) | |||
- Added `Operator Version: X.Y.Z` information in the operator logs.([#1953](https://github.com/operator-framework/operator-sdk/pull/1953)) |
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.
Since we have the release of 0.11.
In order, we are able to merge its required: rebase the PR with the master branch and move this entry for the UNRELEASED section. Could you help us with @drnic? Also, in order to avoid rebase conflicts I would suggest to you keep the update changelog in a another commit, is it possible?
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.
@camilamacedo86 ok, done
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.
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.
Oh you released 0.11 without this PR. Sorry.
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 kept the PR as two commits and fixed/updated your CHANGELOG update commit.
0dfde04
to
1ec794c
Compare
New changes are detected. LGTM label has been removed. |
1ec794c
to
dfa04a9
Compare
/test e2e-aws-go |
/retest Please review the full test history for this PR and help us cut down flakes. |
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.
My only concern here is that I'm not sure how many SDK users are aware of the version
package that gets scaffolded into their projects. However, since this affects only scaffolded code that can be changed by users and only newly scaffolded projects, it isn't really a problem.
LGTM, after fixing nits in CHANGELOG.
Currently an Operator built from scaffold does not print the Operator's version.Version
Co-Authored-By: Camila Macedo <[email protected]>
dfa04a9
to
3ba62e5
Compare
New changes are detected. LGTM label has been removed. |
Description of the change:
The scaffold for Operator bootstrapping now includes the Operator's
version.Version
.Motivation for the change:
Currently an Operator built from scaffold does not print the Operator's
version.Version
(This is my first PR; sorry if there is additional context you need, or if you're not looking for this type of PR)