✨ Clusterctl version check #3484

wfernandes · 2020-08-13T20:13:04Z

What this PR does / why we need it:
This PR allows clusterctl to prompt the user if it detects that the version being used is different from the latest available release. If the clusterctl version being used is either older or a dev release compared to the latest available release, it will print out a message to the terminal via os.Stderr. The version check happens on any sub-command of clusterctl.

An example of the output printed is:

$ clusterctl version
clusterctl version: &version.Info{Major:"0", Minor:"3", GitVersion:"v0.3.8-50-352a3a9d8b5430-dirty", GitCommit:"352a3a9d8b543078d194d80a34a2268652b314a0", GitTreeState:"dirty", BuildDate:"2020-08-13T20:01:57Z", GoVersion:"go1.13.14", Compiler:"gc", Platform:"darwin/amd64"}
Using configuration File="/Users/wfernandes/.cluster-api/clusterctl.yaml"


New clusterctl version available: v0.3.8-50-352a3a9d8b5430-dirty -> v0.3.8
https://github.com/kubernetes-sigs/cluster-api/releases/tag/v0.3.8

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3466

wfernandes · 2020-08-13T20:57:32Z

/test pull-cluster-api-e2e

vincepri · 2020-08-18T16:20:07Z

@wfernandes From your example New clusterctl version available: v0.3.8-50-352a3a9d8b5430-dirty -> v0.3.8, the local built one is actually newer than the released version

wfernandes · 2020-08-18T17:04:21Z

@vincepri Yeah...I realized that actually. I'm still looking into parsing the GitVersion (git describe --tags) into a proper semver.
I found an option: https://github.com/mdomke/git-semver but I'm not sure if I want to add another dependency just for this.
I thought I'd submit another PR for that specifically but if you'd prefer I can work on adding it in this PR.

vincepri · 2020-08-18T17:10:48Z

We should be able to just use semver to compare IIRC

wfernandes · 2020-08-18T17:26:24Z

Yeah so for the example above:
For the version v0.3.8-50-352a3a9d8b5430-dirty,
-50-352a3a9d8b5430-dirty is treated as a pre-release for v0.3.8. And the blang/semver library we are using marks a version v0.3.8 is greater than v0.3.8-50-352a3a9d8b5430-dirty which makes sense IMO.

TBH IMO, it feels that -50-352a3a9d8b5430-dirty should be considered as build metadata and the GitVersion should technically be v0.3.8+50.352a3a9d8b5430.dirty but I'm making sure if my understanding is correct before I make any changes.

vincepri · 2020-08-18T17:44:05Z

Maybe when using pre-release version we should say something like "you're using a pre-release version" or if it's "-dirty" we should say "locally built version"?

cmd/clusterctl/cmd/version_checker.go

wfernandes · 2020-08-18T22:43:09Z

cmd/clusterctl/cmd/version_checker_test.go

+			name: "returns message if cli version is a pre-release of the latest available release",
+			cliVersion: func() version.Info {
+				return version.Info{
+					GitVersion: "v0.3.9-alpha.0",
+				}
+			},
+			githubResponse: `{"tag_name": "v0.3.9", "html_url": "https://github.com/foo/bar/releases/v0.3.9"}`,
+			expectErr:      false,
+			expectedOutput: `
+New clusterctl version available: v0.3.9-alpha.0 -> v0.3.9
+https://github.com/foo/bar/releases/v0.3.9
+`,


If we ever had our own official pre-releases, I wanted clusterctl to be able to notify the user via the output.

wfernandes · 2020-08-18T22:44:50Z

cmd/clusterctl/cmd/version_checker_test.go

+			name: "returns message if cli version is a pre-release and latest available release is the next pre-release",
+			cliVersion: func() version.Info {
+				return version.Info{
+					GitVersion: "v0.3.8-alpha.0",
+				}
+			},
+			githubResponse: `{"tag_name": "v0.3.8-alpha.1", "html_url": "https://github.com/foo/bar/releases/v0.3.8-alpha.1"}`,
+			expectErr:      false,
+			expectedOutput: `
+New clusterctl version available: v0.3.8-alpha.0 -> v0.3.8-alpha.1
+https://github.com/foo/bar/releases/v0.3.8-alpha.1
+`,


Same here. I wanted clusterctl to inform users if there were valid pre-releases available.
As long as we don't name our pre-releases in the same format as GitVersion we should be good.

wfernandes · 2020-08-18T22:48:41Z

cmd/clusterctl/cmd/version_checker.go

+	// if we are using a dirty dev build, just log it out
+	if strings.HasSuffix(cliVer.String(), "-dirty") {
+		log.V(1).Info("Using a dirty dev build of clusterctl.", "CLIVersion", cliVer.String(), "LatestGithubRelease", release.Version)
+		return "", nil
+	}
+
+	// if the cli version is a dev build off of the latest available release,
+	// the just log it out as informational.
+	if strings.HasPrefix(cliVer.String(), latestVersion.String()) && gitVersionRegEx.MatchString(cliVer.String()) {
+		log.V(1).Info("Using a dev build of latest clusterctl release.", "CLIVersion", cliVer.String(), "LatestGithubRelease", release.Version)
+		return "", nil
+	}


I chose to log out here because these are cases specifically for the "developer" users.
"developer" users being folks who have manually built clusterctl.
And I didn't want to bother the developers by printing out the a warning message after every command. TBH, I feel for the majority of users, these use cases will be minimal.
I felt that this was a good balance of notifying a "developer" instead of annoying them with a constant warning message.

cmd/clusterctl/cmd/version_checker.go

vincepri · 2020-08-19T15:23:31Z

cmd/clusterctl/cmd/version_checker.go

+	return "", nil
+}
+
+func (v *versionChecker) getLatestRelease() (*ReleaseInfo, error) {


Suggested change

func (v *versionChecker) getLatestRelease() (*ReleaseInfo, error) {

func (v *versionChecker) getLatestRelease() (ReleaseInfo, error) {

cmd/clusterctl/cmd/version_checker.go

cmd/clusterctl/cmd/version_checker_test.go

cmd/clusterctl/cmd/version_checker.go

vincepri · 2020-08-25T18:59:26Z

@wfernandes Squash?

wfernandes · 2020-08-26T18:03:26Z

@CecileRobertMichon @vincepri From the CAPI meeting, I can implement the strictly blocking feature separately as part of a different issue. Unless y'all wanted that to go in with this work. LMK your thoughts. Thanks.

CecileRobertMichon · 2020-08-26T18:05:00Z

I vote on a separate issue, this is a good incremental change as-is.

wfernandes · 2020-08-26T18:06:58Z

I can create a separate issue for that then and work on it. Thanks for the input.

vincepri

/approve
/milestone v0.3.9
/assign @CecileRobertMichon @fabriziopandini
for final lgtm

cmd/clusterctl/cmd/version_checker.go

CecileRobertMichon · 2020-08-26T23:31:13Z

cmd/clusterctl/cmd/version_checker.go

+	if vs == nil {
+		release, _, err := v.githubClient.Repositories.GetLatestRelease(context.TODO(), "kubernetes-sigs", "cluster-api")
+		if err != nil {
+			log.V(1).Info("⚠️ Unable to get latest github release for clusterctl")


Are we avoiding v(0) to avoid printing logs in the middle of yaml output? otherwise I think it makes sense to print this as a warning on all runs

This was in regards to a previous feedback comment about failing silently. So I thought putting it at a log level that's informative only when asked for. As per the logging guidelines laid out here.

Also since this check is being run after every command, I didn't want the user to be peppered with the log line all the time.

CecileRobertMichon

overall lgtm, a couple of minor comments on formatting and log level

- Store release info in state file Cache the release info in the state file in order to avoid excessive github requests. - Use GITHUB_TOKEN for github client if available - Handle special cases of GitVersions We want to treat pre-releases as valid. We log out if the cli version is a dev build of any kind - Do not return error if unable to get github release This can be the case for air-gapped environments. So we just log the error instead. - Prompt upgrade only within minor versions of cli

CecileRobertMichon

/lgtm

vincepri

/approve

k8s-ci-robot · 2020-08-27T17:05:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

~~cmd/clusterctl/OWNERS~~ [vincepri]

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

vincepri · 2020-08-27T17:05:04Z

/test pull-cluster-api-test

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 13, 2020

k8s-ci-robot requested review from benmoss and CecileRobertMichon August 13, 2020 20:13

wfernandes force-pushed the clusterctl-version-check branch from 2dbf915 to a52ec67 Compare August 13, 2020 20:23

wfernandes commented Aug 18, 2020

View reviewed changes

cmd/clusterctl/cmd/version_checker.go Outdated Show resolved Hide resolved

wfernandes commented Aug 18, 2020

View reviewed changes

wfernandes force-pushed the clusterctl-version-check branch from 4dcb857 to e7c1b31 Compare August 18, 2020 22:46

wfernandes commented Aug 18, 2020

View reviewed changes

vincepri reviewed Aug 19, 2020

View reviewed changes

fabriziopandini reviewed Aug 21, 2020

View reviewed changes

cmd/clusterctl/cmd/version_checker.go Show resolved Hide resolved

cmd/clusterctl/cmd/version_checker.go Outdated Show resolved Hide resolved

cmd/clusterctl/cmd/version_checker.go Outdated Show resolved Hide resolved

wfernandes force-pushed the clusterctl-version-check branch from 06f7723 to 7114511 Compare August 25, 2020 20:12

vincepri approved these changes Aug 26, 2020

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon and fabriziopandini Aug 26, 2020

k8s-ci-robot added this to the v0.3.9 milestone Aug 26, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2020

wfernandes mentioned this pull request Aug 26, 2020

clusterctl should block its usage across minor versions of CAPI #3534

Closed

CecileRobertMichon reviewed Aug 26, 2020

View reviewed changes

cmd/clusterctl/cmd/version_checker.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Aug 26, 2020

View reviewed changes

cmd/clusterctl/cmd/version_checker.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Aug 26, 2020

View reviewed changes

CecileRobertMichon approved these changes Aug 26, 2020

View reviewed changes

wfernandes force-pushed the clusterctl-version-check branch from 7114511 to ef3d157 Compare August 27, 2020 15:57

CecileRobertMichon approved these changes Aug 27, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2020

vincepri approved these changes Aug 27, 2020

View reviewed changes

k8s-ci-robot merged commit b7546f7 into kubernetes-sigs:master Aug 27, 2020

wfernandes deleted the clusterctl-version-check branch August 27, 2020 17:23

wfernandes mentioned this pull request Aug 31, 2020

Remove hard coded info about cert manager #3505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Clusterctl version check #3484

✨ Clusterctl version check #3484

wfernandes commented Aug 13, 2020 •

edited

Loading

wfernandes commented Aug 13, 2020

vincepri commented Aug 18, 2020

wfernandes commented Aug 18, 2020

vincepri commented Aug 18, 2020

wfernandes commented Aug 18, 2020

vincepri commented Aug 18, 2020

wfernandes Aug 18, 2020

wfernandes Aug 18, 2020

wfernandes Aug 18, 2020

vincepri Aug 19, 2020

vincepri commented Aug 25, 2020

wfernandes commented Aug 26, 2020

CecileRobertMichon commented Aug 26, 2020

wfernandes commented Aug 26, 2020

vincepri left a comment

CecileRobertMichon Aug 26, 2020

wfernandes Aug 27, 2020

CecileRobertMichon left a comment

CecileRobertMichon left a comment

vincepri left a comment

k8s-ci-robot commented Aug 27, 2020

vincepri commented Aug 27, 2020

	func (v versionChecker) getLatestRelease() (ReleaseInfo, error) {
	func (v *versionChecker) getLatestRelease() (ReleaseInfo, error) {

✨ Clusterctl version check #3484

✨ Clusterctl version check #3484

Conversation

wfernandes commented Aug 13, 2020 • edited Loading

wfernandes commented Aug 13, 2020

vincepri commented Aug 18, 2020

wfernandes commented Aug 18, 2020

vincepri commented Aug 18, 2020

wfernandes commented Aug 18, 2020

vincepri commented Aug 18, 2020

wfernandes Aug 18, 2020

Choose a reason for hiding this comment

wfernandes Aug 18, 2020

Choose a reason for hiding this comment

wfernandes Aug 18, 2020

Choose a reason for hiding this comment

vincepri Aug 19, 2020

Choose a reason for hiding this comment

vincepri commented Aug 25, 2020

wfernandes commented Aug 26, 2020

CecileRobertMichon commented Aug 26, 2020

wfernandes commented Aug 26, 2020

vincepri left a comment

Choose a reason for hiding this comment

CecileRobertMichon Aug 26, 2020

Choose a reason for hiding this comment

wfernandes Aug 27, 2020

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 27, 2020

vincepri commented Aug 27, 2020

wfernandes commented Aug 13, 2020 •

edited

Loading