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.

Sign up for GitHub

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

Jump to bottom

Support reading manifest url from files #429

Merged
merged 3 commits into from
Dec 28, 2019
Merged

Support reading manifest url from files #429

merged 3 commits into from
Dec 28, 2019

Conversation

ahmetb
Copy link
Member

@ahmetb ahmetb commented Dec 9, 2019 •
edited

Addressing some technical debt:

  • Introduce indexscanner.ReadPlugin(io.ReadCloser)
  • Make ReadPluginFromFile use that to simplify
  • Move validation.ValidatePlugin from several places to ReadPlugin() method
  • Add missing test for "preserves ENOENT" behavior for ReadPluginFromFile
  • Fix verify-gofmt.sh reference (now gone) in run-tests.sh

Adding new stuff:

  • introduce --manifest-url that is mut.ex. with --manifest and positional args
  • add unit tests for url fetching
  • add integration tests for --manifest-url argument behavior

Addresses #193, potentially supersedes #350.

@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 Dec 9, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 9, 2019
@ahmetb ahmetb mentioned this pull request Dec 9, 2019
Closed
@codecov-io
Copy link

codecov-io commented Dec 9, 2019 •
edited

Codecov Report

Merging #429 into master will increase coverage by 0.29%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   56.25%   56.54%   +0.29%     
==========================================
  Files          19       19              
  Lines         928      925       -3     
==========================================
+ Hits          522      523       +1     
+ Misses        351      349       -2     
+ Partials       55       53       -2
Impacted Files Coverage Δ
internal/info/info.go 100% <100%> (ø) ⬆️
internal/installation/receipt/receipt.go 75% <100%> (ø) ⬆️
cmd/validate-krew-manifest/main.go 54.08% <100%> (ø) ⬆️
internal/index/indexscanner/scanner.go 72.54% <81.81%> (+5.88%) ⬆️

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 02ceef2...28483b3. Read the comment docs.

@corneliusweig corneliusweig self-assigned this Dec 9, 2019
corneliusweig
corneliusweig suggested changes Dec 9, 2019
View reviewed changes
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.

Very clean work! Just a few nits.

integration_test/install_test.go Outdated Show resolved Hide resolved
internal/index/indexscanner/scanner.go Show resolved Hide resolved
internal/index/indexscanner/scanner.go Show resolved Hide resolved
@ahmetb ahmetb mentioned this pull request Dec 10, 2019
Merged
corneliusweig
corneliusweig reviewed Dec 23, 2019
View reviewed changes
integration_test/install_test.go Outdated
Comment on lines 115 to 116
srv, close := localTestServer()
defer close()
Copy link
Contributor

@corneliusweig corneliusweig Dec 23, 2019 •
edited

Choose a reason for hiding this comment

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

I'm not exactly sure what it is shadowing, but gocritic is unhappy about calling it close. (same below)

Copy link
Member Author

Choose a reason for hiding this comment

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

shadows close builtin method on go channels :)

Lets use cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

💡 cleanup sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

@ahmetb
Support reading manifest url from files
2bc96d4 
Addressing some technical debt:

- Introduce indexscanner.ReadPlugin(io.ReadCloser)
- Make ReadPluginFromFile use that to simplify
- Move validation.ValidatePlugin from several places to ReadPlugin() method
- Add missing "preserves ENOENT" error for ReadPluginFromFile
- Fix verify-gofmt.sh reference (now gone) in run-tests.sh

Adding new stuff:

- introduce --manifest-url that is mut.ex. with --manifest and positional args
- add unit tests for url fetching
- add integration tests for --manifest-url argument behavior

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb
fix TestKrewInstall_ManifestArgsAreMutuallyExclusive
720a0e3 
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb
close() -> shutdown()
28483b3 
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb
Copy link
Member Author

ahmetb commented Dec 27, 2019

Should be good to go.

@corneliusweig
Copy link
Contributor

/lgtm
/approve
Sorry for the delay 🙏

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 28, 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 68c452b into kubernetes-sigs:master Dec 28, 2019
@grosser grosser mentioned this pull request Dec 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corneliusweig corneliusweig corneliusweig requested changes

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@corneliusweig corneliusweig

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ahmetb @codecov-io @corneliusweig @k8s-ci-robot