Check for new krew versions once a day #490

corneliusweig · 2020-02-02T00:48:15Z

Fixes #480

This will check once a day if a new version of krew is available.
The timestamp of the last run is stored in KREW_HOME/last_update_check.
As this is a convenience function, silently ignore errors if they occur.

To opt out of this feature, set the KREW_NO_UPGRADE_CHECK env variable.

This will check once a day if a new version of krew is available. The timestamp of the last run is stored in KREW_HOME/last_update_check. As this is a convenience function, silently ignore errors if they occur.

k8s-ci-robot · 2020-02-02T00:48:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: corneliusweig

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~~ [corneliusweig]

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

codecov-io · 2020-02-02T00:54:09Z

Codecov Report

Merging #490 into master will increase coverage by 0.55%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage   56.47%   57.03%   +0.55%     
==========================================
  Files          19       20       +1     
  Lines         926      966      +40     
==========================================
+ Hits          523      551      +28     
- Misses        350      357       +7     
- Partials       53       58       +5

Impacted Files	Coverage Δ
internal/updatecheck/update.go	`70% <70%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7842e57...8466e32. Read the comment docs.

cmd/krew/cmd/root.go

docs/USER_GUIDE.md

internal/assertion/update.go

ahmetb · 2020-02-02T03:29:56Z

internal/assertion/update.go

+
+func saveTimestamp(file string) {
+	klog.V(4).Info("Saving timestamp for last version check")
+	timestamp := time.Now().Format(time.RFC1123)


why not just use unix time as the persisted format?

Do you mean seconds since epoch? I prefer a more readable format. I changed to something which still easy to manipulate.

internal/assertion/update.go

This code is racey now, because the global variable is accessed without synchronization. Since this is a convenience feature, this is acceptable.

- Improve code structure - Wrap errors instead of logging - Improve wording of upgrade message

- Improve code structure - Wrap errors instead of logging - Improve wording of upgrade message - Change timestamp format

cmd/krew/cmd/root.go

ahmetb · 2020-02-03T01:03:39Z

internal/updatecheck/update.go


-	klog.V(4).Infof("Found latest tag %q", latestTag)
-	return latestTag
+	if version.GitTag() == latestTag {


I think it would be nicer to actually use our semver pkg to do a > check here.

or rather, maybe don't do the version tag here?

I feel like the CheckVersion isn't matching what this method does with reading last_update_check time + comparison with current version.

I'd rather not do any semver parsing here. In tests, version.GitTag returns unknown which would not work.
Besides, we only allow the remote version to increase. So checking for equality also does the job.

ahmetb · 2020-02-03T17:30:51Z

internal/updatecheck/update.go

+	}
+	saveTimestamp(f)
+
+	if version.GitTag() == latestTag {


also worth keeping in mind that sometimes version.GitTag() might return malformed or empty string that isn't a semver.

if we actually parse that higher in the stack, we don't even have to fetchLatestTag().

Do you mean to say that the update check should not run for development builds? That would make perfect sense.

Why not? If I want to be in the edge, I would care updating even more than a regular user.

Why not:

if you are developing krew, you are usually ahead of the latest release. So the "upgrade" notification would recommend a downgrade :)

the CI runs non-release builds of krew in integration tests. We don't need upgrade notifications there.

Do you mean to say that the update check should not run for development builds? That would make perfect sense.

Not just development builds.
For example, until recently homebrew formula did not have -ldflags that stamp the GitTag into the binary.
Also think of people running go get sigs.k8s.io/krew/cmd/krew, it will not have a GitTag in it.

For those cases, just fail to parse version as semver, and move on.

if you are developing krew, you are usually ahead of the latest release. So the "upgrade" notification would recommend a downgrade :)

thats why (1) parse strings into SemVer (2) use semver.LessThan to compare instead of !=.

ahmetb · 2020-02-03T17:33:14Z

internal/updatecheck/update.go

+	if err != nil {
+		return "", errors.Wrapf(err, "could not determine latest tag")
+	}
+	saveTimestamp(f)


I think we need defer saveTimestamp() earlier in this func.

last_update_check shouldn't just count successful checks, but rather "completed attempts".

Why is that? If a check fails, we should retry soon instead of waiting the full cool-down. So without defer sounds more reasonable to me.

ahmetb · 2020-02-03T17:35:45Z

So right now we check every 24 hours.
And we record "checked" status only if:

request completed before krew does its job (probably not for krew version, krew info, krew search etc) AND
request was successful

we need to be careful about the impact of this.

ahmetb · 2020-02-03T18:30:18Z

internal/updatecheck/update_test.go

+	}
+}
+
+func Test_fetchTag(t *testing.T) {


actually something more valuable to test than "github returning broken json" would be "github unreachable" case.

it's far more likely to happen (Little Snitch, and other corp firewalls etc.)

I agree, but I don't see how I can simulate this situation in tests. From the docs of http.Get:

An error is returned if there were too many redirects or if there
was an HTTP protocol error.

So I don't see how I could possibly cause an error here :)

Ah well.. I can just use a bad address, which will cause an HTTP error.

ahmetb · 2020-02-03T19:26:41Z

internal/updatecheck/update.go

+		klog.V(3).Info("Latest tag is same as ours.")
+		return "", nil
+	}
+	return color.New(color.Bold).Sprintf(upgradeNotification, version.GitTag(), latestTag), nil


I think this method doesn't have a good signature.
Returning strings with colors isn't great.

let's change it to something like this

func UpdateAvailable(basePath string, currentVersion semver.Version) (semver.Version, exists bool, error)

then we can curate the message outside.

ahmetb · 2020-02-03T19:28:28Z

internal/updatecheck/update_test.go

+	}
+}
+
+func Test_fetchTag(t *testing.T) {


can we add a TestFetchTag that actually queries GitHub?

I know making network calls isn't a great idea during unit tests, but it would help us notice if the Github API breaks somehow under us.

corneliusweig · 2020-02-03T20:53:55Z

we need to be careful about the impact of this.

If your really think we could run into problems, let's reset the cooldown also after failed API requests. As long as the cooldown is not too large, this is fine with me.

ferhatelmas · 2020-02-03T21:20:58Z

internal/updatecheck/update.go

+	content, err := ioutil.ReadFile(file)
+	if err != nil {
+		klog.V(4).Infof("Could not read timestamp file %q", file)
+		return time.Unix(0, 0)


why not default? time.Time{}?

Since it’s not supposed to be used when err exists, it practically doesn’t matter.

I was wondering if there was a special reason.

Yes, end result doesn't change but it's like []type vs nil, would be more idiomatic.

ferhatelmas · 2020-02-03T21:23:46Z

internal/updatecheck/update.go

+// will only be emitted once a day.
+func CheckVersion(basePath string) (string, error) {
+	f := filepath.Join(basePath, "last_update_check")
+	if lastCheck := loadTimestamp(f); time.Since(lastCheck).Hours() <= 24 {


could you put the range to the top as constant?

an idea might be introducing another environment variable to customize it.

Just hardcoded const is good

ahmetb · 2020-02-04T01:09:37Z

If your really think we could run into problems, let's reset the cooldown also after failed API requests.

on the contrary, if we fail, I think that still should count as a "completed attempt" –and we should not do any more requests in the next 24h IMO.

We want people to eventually update; not right away. However, if a check isn't complete (i.e. krew exists before we can request+print warning), we should not record that and run it again next time.

ferhatelmas · 2020-02-04T09:50:31Z

docs/USER_GUIDE.md

+the `KREW_NO_UPGRADE_CHECK` environment variable. To permanently disable this,
+add the following to your `~/.bashrc`, `~/.bash_profile`, or `~/.zshrc`:
+
+    export KREW_NO_UPGRADE_CHECK=1


An idea: since we don't know how it will affect users in the beginnning, we might make it opt-in to use at first.

-1. we discussed at #480 that this isn't sending PII. It's basically almost like a git clone that we already do. I don't wish users to learn yet another thing. Let's keep the interaction surface minimal.

I agree, sounds good.

Btw, sorry for skipping the actual discussion and opening them here again.

ahmetb · 2020-02-04T20:47:37Z

docs/USER_GUIDE.md

@@ -104,6 +104,15 @@ without any arguments:
 Since `krew` itself is a plugin also managed through `krew`, running the upgrade
 command may also upgrade your `krew` version.

+### Krew upgrade check


This is too high up.

Let's move this below to the main user journey and call it ## Disabling update checks

corneliusweig · 2020-02-04T22:05:34Z

A major part of the discussion has revolved around the file where the timestamp of the last check is stored. I created an alternate version of this feature where checks are done at random here #494.

So I'll close this if favor of the other PR.
/close

k8s-ci-robot · 2020-02-04T22:05:36Z

@corneliusweig: Closed this PR.

In response to this:

A major part of the discussion has revolved around the file where the timestamp of the last check is stored. I created an alternate version of this feature where checks are done at random here #494.

So I'll close this if favor of the other PR.
/close

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.

corneliusweig added 4 commits February 2, 2020 01:30

Add utils to check for krew updates

331cf57

This will check once a day if a new version of krew is available. The timestamp of the last run is stored in KREW_HOME/last_update_check. As this is a convenience function, silently ignore errors if they occur.

Show notifications right before the command completes

e2c4afe

Add switch to disable the upgrade check

01a1af9

Minor simplifications

5bee68a

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 2, 2020

k8s-ci-robot requested review from ahmetb and soltysh February 2, 2020 00:48

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

Fix linter issue: explicitly ignore return values

cd6246f