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

Add --no-update-index option to bypass updates #279

Merged

Conversation

ahmetb
Copy link
Member

@ahmetb ahmetb commented Jul 18, 2019

This option added to 'install' and 'upgrade' sub-commands allows users
(primarily developers) to test behavior locally (such as upgrades) while
preventing krew from auto-{updating,reverting,resetting} changes to the
local copy of the index repository at $KREW_ROOT/index.

Introducing this flag as "experimental", this way we can delete it if we don't
like it in the future. It's provided as an "escape hatch". An alternative I
can think of is a file like $dir/.noupdate existing in the index repo, but
I think this is cleaner.

Two issues that are in my mind currently wrt this change:

  1. I can't recide between --no-update-index vs --no-index-update. In our
    gcloud conventions we tend to use --no-$VERB-$NOUN so I'm going with
    this.

  2. Writing integration tests currently sound unnecessary to me, although it's
    probably possible (we can write files to the per-test index dir, and see if
    they survive krew upgrade and krew install, which don't have to succeed).
    Thoughts?

Fixes #263.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 18, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 18, 2019
@codecov-io
Copy link

codecov-io commented Jul 18, 2019

Codecov Report

Merging #279 into master will increase coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
+ Coverage   55.07%   55.34%   +0.26%     
==========================================
  Files          19       19              
  Lines         877      880       +3     
==========================================
+ Hits          483      487       +4     
+ Misses        341      337       -4     
- Partials       53       56       +3
Impacted Files Coverage Δ
pkg/installation/install.go 32% <0%> (-2.32%) ⬇️
pkg/index/validate.go 95% <0%> (-0.35%) ⬇️
pkg/installation/upgrade.go 0% <0%> (ø) ⬆️
pkg/installation/util.go 48.88% <0%> (+7.5%) ⬆️

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 44caf6c...88fec4a. Read the comment docs.

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, otherwise looks good.

glog.V(4).Infof("--manifest specified, not ensuring plugin index")
return nil
if *noUpdateIndex {
glog.V(4).Infof("--no-update-index specified, skipping updating local copy of plugin index")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a return nil here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed.

This option added to 'install' and 'upgrade' sub-commands allows users
(primarily developers) to test behavior locally (such as upgrades) while
preventing krew from auto-{updating,reverting,resetting} changes to the
local copy of the index repository at $KREW_ROOT/index.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@corneliusweig
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2019
@k8s-ci-robot
Copy link
Contributor

[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 2364ae4 into kubernetes-sigs:master Jul 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: introduce a way to bypass index updates
4 participants