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

Randomly check for krew updates #494

Merged
merged 8 commits into from Feb 7, 2020

Conversation

@corneliusweig
Copy link
Contributor

corneliusweig commented Feb 4, 2020

Fixes #480
Related issue: #490

This is an alternate implementation of #490. The goal here is to simplify the feature. Instead of saving the timestamp of the last update check, the checks are now done at random with a probability of 40% per run. In addition, no checks are done for development builds.

As this is a convenience function, silently ignore errors if they occur.
Show notifications right before the command completes
Add env var KREW_NO_UPGRADE_CHECK as switch to disable the upgrade check.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 4, 2020

Codecov Report

Merging #494 into master will increase coverage by 1.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #494     +/-   ##
=========================================
+ Coverage      57%   58.21%   +1.2%     
=========================================
  Files          21       22      +1     
  Lines         956      974     +18     
=========================================
+ Hits          545      567     +22     
+ Misses        357      351      -6     
- Partials       54       56      +2
Impacted Files Coverage Δ
cmd/krew/cmd/internal/fetch_tag.go 100% <100%> (ø)
internal/installation/util.go 50% <0%> (+50%) ⬆️

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 7ec26f6...1b24548. Read the comment docs.

const (
upgradeNotification = `A newer version of krew is available (%s -> %s).
Run "kubectl krew upgrade" to get the newest version!
`

This comment has been minimized.

Copy link
@ahmetb

ahmetb Feb 4, 2020

Member

maybe remove newline and use println?

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Feb 5, 2020

Author Contributor

Println is not possible due to the format string. I changed to a normal string instead.

cmd/krew/cmd/root.go Outdated Show resolved Hide resolved
cmd/krew/cmd/root.go Outdated Show resolved Hide resolved
docs/USER_GUIDE.md Outdated Show resolved Hide resolved
internal/updatecheck/update.go Outdated Show resolved Hide resolved
// See the License for the specific language governing permissions and
// limitations under the License.

package updatecheck

This comment has been minimized.

Copy link
@ahmetb

ahmetb Feb 4, 2020

Member

I don't know why you want to make a new pkg out of this tbh :)

it's like 3 short methods.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Feb 4, 2020

Author Contributor

Yeah.. true. I'll move to cmd/internal

internal/updatecheck/update.go Outdated Show resolved Hide resolved
@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Feb 4, 2020

No major issues just some nits+refactoring feedback.

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

corneliusweig commented Feb 5, 2020

Ok, I hope I got everything.

As this is a convenience function, silently ignore errors if they occur.
Show notifications right before the command completes
Add env var KREW_NO_UPGRADE_CHECK as switch to disable the upgrade check.
@corneliusweig corneliusweig force-pushed the corneliusweig:w/PATH-check-2 branch from a95789a to 956e101 Feb 5, 2020
cmd/krew/cmd/root.go Outdated Show resolved Hide resolved
return nil
}

func showUpgradeNotification(*cobra.Command, []string) {
if latestTag == "" || latestTag == version.GitTag() {

This comment has been minimized.

Copy link
@ahmetb

ahmetb Feb 5, 2020

Member

I think I've mentioned this before: Let's parse both of these into semver, and actually do LessThan check.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Feb 5, 2020

Author Contributor

Yes, I also explained why I think that mere string comparison is better:

We only allow the version numbers of krew releases to advance, so that it is equivalent to check for string equality vs. semver less-than comparison.

But if you feel so strongly about it, let's change it.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Feb 5, 2020

Member

I feel like we're gonna see upgrade notifications etc in CI tests.
If it's not a big deal, let's just do that. We already have the code, and we'd simply skip if it fails to parse as semver.

Comments
Use semver.Less to compare version tags
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Feb 6, 2020

Added a commit PTAL.

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

corneliusweig commented Feb 6, 2020

Oh yes, make sense. Thanks!
There was a small golint problem, let's see if it is clean now.

@ahmetb ahmetb closed this Feb 7, 2020
@ahmetb ahmetb reopened this Feb 7, 2020
@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Feb 7, 2020

(Closed by mistake) 🗡

I've encountered this: On local builds, it prints

A newer version of krew is available (v0.3.3-52-gf66516f -> v0.3.3).
Run "kubectl krew upgrade" to get the newest version!

I guess this is expected, because, technically, pre-release tags are LessThan their tags. 🤦‍♂ What do we do?

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Feb 7, 2020

I guess this problem only impact developers. I hope the devs can understand what's going on. If you have any solutions, please send follow-up PR.

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link

k8s-ci-robot commented Feb 7, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, 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 [ahmetb,corneliusweig]

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 20c0e20 into kubernetes-sigs:master Feb 7, 2020
4 of 5 checks passed
4 of 5 checks passed
build
Details
build
Details
tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation corneliusweig authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@corneliusweig corneliusweig deleted the corneliusweig:w/PATH-check-2 branch Feb 7, 2020
@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

corneliusweig commented Feb 7, 2020

Let's see if annoying this is to us. I'd like to avoid complexity if it only solves a minor inconvenience for a few krew developers.

Btw, thanks for pushing this PR in the right direction until it became reasonably small!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.