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 OS/Arch information in the krew version CLI output #484

Merged
merged 1 commit into from
Feb 2, 2020

Conversation

tony-yang
Copy link
Contributor

@tony-yang tony-yang commented Jan 30, 2020

Add OS/Arch information in the krew version CLI output to help with diagnostics.
No unit test added since the function we are calling is already well tested.

Updated the KrewVersionTest integration test to convert the stdout to go map to leverage go-cmp for better test assertion. We now can report failure when new output is added to the krew version command.

Fixes #470

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 30, 2020
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 30, 2020
@tony-yang
Copy link
Contributor Author

Reference issue: #470

@tony-yang
Copy link
Contributor Author

/assign @soltysh

@ahmetb
Copy link
Member

ahmetb commented Jan 30, 2020

/unassign @soltysh

@tony-yang do you mind adding a test to TestKrewVersion* ?

@tony-yang
Copy link
Contributor Author

tony-yang commented Jan 30, 2020

/unassign @soltysh

@tony-yang do you mind adding a test to TestKrewVersion* ?

Sure, will update the PR later

@tony-yang tony-yang changed the title Add OS/Arch information in the krew version CLI output [WIP] Add OS/Arch information in the krew version CLI output Jan 30, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2020
@ahmetb
Copy link
Member

ahmetb commented Jan 30, 2020

Thanks.

cmd/krew/cmd/version.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 1, 2020

Codecov Report

Merging #484 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #484   +/-   ##
=======================================
  Coverage   56.47%   56.47%           
=======================================
  Files          19       19           
  Lines         926      926           
=======================================
  Hits          523      523           
  Misses        350      350           
  Partials       53       53

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 5ceeae5...e688a44. Read the comment docs.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 1, 2020
@tony-yang tony-yang changed the title [WIP] Add OS/Arch information in the krew version CLI output Add OS/Arch information in the krew version CLI output Feb 1, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2020
@tony-yang
Copy link
Contributor Author

Hi, @ahmetb I updated the integration test.
Just wondering, should I use the assign function for this project (since I saw you removed it earlier)? Thanks.

@ahmetb
Copy link
Member

ahmetb commented Feb 1, 2020

Which assign function are you referring to?

@corneliusweig
Copy link
Contributor

corneliusweig commented Feb 1, 2020

Hi @tony-yang, /assign is fine, you only tagged the wrong person 😉. But it's usually not necessary in krew as we stay on top of PRs and issues.

@tony-yang
Copy link
Contributor Author

Hi @tony-yang, /assign is fine, you only tagged the wrong person 😉. But it's usually not necessary in krew as we stay on top of PRs and issues.

Thanks Cornelius, good to know.

@ahmetb
Copy link
Member

ahmetb commented Feb 2, 2020

/lgtm
/approve
ty!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, tony-yang

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit 58a5184 into kubernetes-sigs:master Feb 2, 2020
@tony-yang tony-yang deleted the os-arch branch February 16, 2020 07:51
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show detected os/arch in version
6 participants