-
Notifications
You must be signed in to change notification settings - Fork 474
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
internal/version: Adopt Go 1.18+ implementation #10646
internal/version: Adopt Go 1.18+ implementation #10646
Conversation
Signed-off-by: timflannagan <[email protected]>
Signed-off-by: timflannagan <[email protected]>
Signed-off-by: timflannagan <[email protected]>
Signed-off-by: timflannagan <[email protected]>
- Required to populate VCS metadata in runtime.ReadBuildInfo calls correctly Signed-off-by: timflannagan <[email protected]>
Signed-off-by: timflannagan <[email protected]>
@@ -29,6 +28,7 @@ require ( | |||
github.com/solo-io/go-list-licenses v0.1.4 | |||
github.com/solo-io/go-utils v0.27.3 | |||
github.com/spf13/afero v1.11.0 | |||
github.com/spf13/cobra v1.8.1 |
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'm typically hesitant introducing new direct dependencies, but in this case cobra is pretty standard and I wasn't able to get to refactoring the CLI packages when we were pushing through the deflate/cleanup repository stream of work at the time.
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.
not opposed, but cobra here for a single binary startup (i.e. not a CLI) does seem a bit excessive.
also, we have the Settings
replacement plumbed via env vars, if we are going cobra maybe we should reevaluate Viper for the env var config?
no real blocker here, just thinking out loud
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.
Talked with Yuval about this offline: I was planning on refactoring the SDS/envoyinit cmd packages to use cobra to be consistent with the kgateway package as a follow-up when I have some downtime. In this case, we'd pipe configuration to our internal packages via cobra flags and expose knobs in the helm chart for higher-level config.
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.
@lgadban Glossed over your "not a CLI" comment. IMHO, all these binaries should be CLI-based with the sole responsibility that they 1.) provide flags/configuration to libraries, 2.) call that libaries' constructor and delegate to that library to do the heavy lifting, and 3.) handle any I/O that doesn't make sense in the context of the library being utilized.
just tried it:
looks good! |
@yuval-k Perfect. Last I checked the controller will only log the version.Version variable, but we can adjust that the entire version.String() (which includes the VCS metadata) is logged too. |
Signed-off-by: timflannagan <[email protected]>
Signed-off-by: timflannagan <[email protected]>
@timflannagan can you add a ref link to the PR description for above? |
@lgadban Done, added a link to https://tip.golang.org/doc/go1.18#debug_buildinfo. |
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.
very nice, just a quick discussion point
@@ -29,6 +28,7 @@ require ( | |||
github.com/solo-io/go-list-licenses v0.1.4 | |||
github.com/solo-io/go-utils v0.27.3 | |||
github.com/spf13/afero v1.11.0 | |||
github.com/spf13/cobra v1.8.1 |
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.
not opposed, but cobra here for a single binary startup (i.e. not a CLI) does seem a bit excessive.
also, we have the Settings
replacement plumbed via env vars, if we are going cobra maybe we should reevaluate Viper for the env var config?
no real blocker here, just thinking out loud
Description
This PR overhauls the pkg/version library:
See the Go 1.18 release notes for more information.
Checklist: