Fix client version output with --component option #1408

anshulvermapatel · 2021-06-29T18:32:44Z

Resolves #1395

Resolved an issue where the client version with --component option was being
printed same as the output of client version without adding --component

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Run the code checkers with make check
Regenerate the manpages, docs and go formatting with make generated
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

tekton-robot · 2021-06-29T18:32:58Z

Hi @anshulvermapatel. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

pradeepitm12 · 2021-06-29T18:33:40Z

/ok-to-test

tekton-robot · 2021-06-29T18:36:24Z

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/version/version.go	64.8%	63.4%	-1.4

chmouel · 2021-06-29T20:20:19Z

I think we need to test in this command, or move the logic block out of Command in a package in pkg/version for example to better make it testable.

vdemeester

One comment 👼🏼

vdemeester · 2021-06-30T11:02:20Z

pkg/cmd/version/version.go

+				if component == "" {
+					fmt.Fprintf(cmd.OutOrStdout(), "Client version: %s\n", clientVersion)
+				} else {
+					fmt.Fprintf(cmd.OutOrStdout(), "%s\n", clientVersion)


I don't think this is right though, because as is, tkn version --component=pipeline with a error connecting kubernetes, it will print the client version without an error, which doesn't make sense to me.

We should print the client version if the component is client, otherwise we should either error out or print unknown.

Yes, my bad.
I will correct it.

anshulvermapatel · 2021-07-05T09:10:04Z

@chmouel , made the tests for this better .. still kept the block in cmd itself as it was a small change. Please do suggest once your review.

tekton-robot · 2021-07-05T09:10:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/version/version.go	64.8%	73.7%	8.8

anshulvermapatel · 2021-07-05T09:45:40Z

/retest

Resolved an issue where the client version as component was being printed as the output of client version without adding `--component`

tekton-robot · 2021-07-05T10:15:48Z

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/version/version.go	64.8%	73.7%	8.8

vinamra28 · 2021-07-08T09:06:49Z

pkg/cmd/version/version_test.go

+				}
+				got, _ := test.ExecuteCommand(version, "version", "-n", "test", "--component", c)
+				fmt.Println(t.Name())
+				golden.Assert(t, got, fmt.Sprintf("%s-%s.golden", t.Name(), c))


instead of generating golden files can't we just do assert?

I tend to agree with @vinamra28, could be a follow-up though 😉

sure. I'll track this in an issue and merge this one 🙂

tekton-robot · 2021-07-08T09:34:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

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

vinamra28 · 2021-07-08T09:43:14Z

/lgtm

vinamra28 · 2021-07-08T09:59:21Z

/retest

vinamra28 · 2021-07-08T10:17:14Z

/retest

piyush-garg · 2021-07-09T06:29:49Z

/lgtm

piyush-garg · 2021-07-09T06:30:10Z

/test pull-tekton-cli-integration-tests

piyush-garg · 2021-07-09T08:48:44Z

/retest

#1409 | [Pradeep Kumar] update readme cli version to v0.19.1 | 2021/06/30-15:54 #1410 | [laminar] Fix CLI installation instructions for deb-based distros | 2021/07/05-10:15 #1393 | [vinamra28] Get tekton components version from configmap | 2021/07/07-09:09 #1411 | [vinamra28] Bump pipelines v0.25.0, triggers 0.14.2 and hub | 2021/07/07-12:05 #1415 | [Vincent Demeester] Force cancel when using tkn pipelinerun cancel 👼 | 2021/07/09-09:48 #1408 | [Anshul Verma] resloves issue/1395 | 2021/07/09-10:12 #1385 | [Nikhil Thomas] Fix TaskRun order in `tkn pr logs` | 2021/07/13-06:13 Signed-off-by: Pradeep Kumar <[email protected]>

tektoncd#1409 | [Pradeep Kumar] update readme cli version to v0.19.1 | 2021/06/30-15:54 tektoncd#1410 | [laminar] Fix CLI installation instructions for deb-based distros | 2021/07/05-10:15 tektoncd#1393 | [vinamra28] Get tekton components version from configmap | 2021/07/07-09:09 tektoncd#1411 | [vinamra28] Bump pipelines v0.25.0, triggers 0.14.2 and hub | 2021/07/07-12:05 tektoncd#1415 | [Vincent Demeester] Force cancel when using tkn pipelinerun cancel 👼 | 2021/07/09-09:48 tektoncd#1408 | [Anshul Verma] resloves issue/1395 | 2021/07/09-10:12 tektoncd#1385 | [Nikhil Thomas] Fix TaskRun order in `tkn pr logs` | 2021/07/13-06:13 Signed-off-by: Pradeep Kumar <[email protected]>

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 29, 2021

tekton-robot requested review from chmouel and hrishin June 29, 2021 18:32

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 29, 2021

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 29, 2021

anshulvermapatel mentioned this pull request Jun 29, 2021

Client version command doesn't work on 0.19.0 release #1395

Closed

vdemeester reviewed Jun 30, 2021

View reviewed changes

anshulvermapatel force-pushed the issue/1395/client_version_issue branch from 2da4451 to 37f5b9c Compare July 5, 2021 09:08

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 5, 2021

resloves issue/1395

8808c7e

Resolved an issue where the client version as component was being printed as the output of client version without adding `--component`

anshulvermapatel force-pushed the issue/1395/client_version_issue branch from 37f5b9c to 8808c7e Compare July 5, 2021 10:13

vdemeester approved these changes Jul 5, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2021

vinamra28 reviewed Jul 8, 2021

View reviewed changes

vdemeester approved these changes Jul 8, 2021

View reviewed changes

tekton-robot assigned vinamra28 Jul 8, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021

vinamra28 mentioned this pull request Jul 8, 2021

Remove extra golden files generated while testing version command #1413

Closed

tekton-robot assigned piyush-garg Jul 9, 2021

tekton-robot merged commit 56c592e into tektoncd:main Jul 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix client version output with --component option #1408

Fix client version output with --component option #1408

anshulvermapatel commented Jun 29, 2021 •

edited

Loading

tekton-robot commented Jun 29, 2021

pradeepitm12 commented Jun 29, 2021

tekton-robot commented Jun 29, 2021

chmouel commented Jun 29, 2021

vdemeester left a comment

vdemeester Jun 30, 2021

anshulvermapatel Jul 1, 2021

anshulvermapatel commented Jul 5, 2021

tekton-robot commented Jul 5, 2021

anshulvermapatel commented Jul 5, 2021

tekton-robot commented Jul 5, 2021

vinamra28 Jul 8, 2021

vdemeester Jul 8, 2021

vinamra28 Jul 8, 2021

tekton-robot commented Jul 8, 2021

vinamra28 commented Jul 8, 2021

vinamra28 commented Jul 8, 2021

vinamra28 commented Jul 8, 2021

piyush-garg commented Jul 9, 2021

piyush-garg commented Jul 9, 2021

piyush-garg commented Jul 9, 2021

Fix client version output with --component option #1408

Fix client version output with --component option #1408

Conversation

anshulvermapatel commented Jun 29, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 29, 2021

pradeepitm12 commented Jun 29, 2021

tekton-robot commented Jun 29, 2021

chmouel commented Jun 29, 2021

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Jun 30, 2021

Choose a reason for hiding this comment

anshulvermapatel Jul 1, 2021

Choose a reason for hiding this comment

anshulvermapatel commented Jul 5, 2021

tekton-robot commented Jul 5, 2021

anshulvermapatel commented Jul 5, 2021

tekton-robot commented Jul 5, 2021

vinamra28 Jul 8, 2021

Choose a reason for hiding this comment

vdemeester Jul 8, 2021

Choose a reason for hiding this comment

vinamra28 Jul 8, 2021

Choose a reason for hiding this comment

tekton-robot commented Jul 8, 2021

vinamra28 commented Jul 8, 2021

vinamra28 commented Jul 8, 2021

vinamra28 commented Jul 8, 2021

piyush-garg commented Jul 9, 2021

piyush-garg commented Jul 9, 2021

piyush-garg commented Jul 9, 2021

anshulvermapatel commented Jun 29, 2021 •

edited

Loading