Modify tkn version to accept namespace as ldflag and flag #1092

savitaashture · 2020-07-31T11:28:58Z

Changes

Issue: Authenticated user(non-admin) cannot get the tkn version info because of permission issue as tkn version generally looks pipeline and triggers controller deployment across the cluster(all-namespaces) and non-admin user generally doesn't have permission to view on all namespaces.

So to satisfy admin and non-admin requirements to get the tkn-version come up with the below solution from WG meeting

Provide namespace as ldflag during tkn compilation
User can specify namespace as flag tkn version -n {namespace}
default namespace will be tekton-pipelines if no ldflag and -n flag specified

Output:

Specified namespace in ldflag is tekton-pipelines1 and controllers are running in tekton-pipelines

$ GOOS=linux GOARCH=amd64 go build -mod=vendor -ldflags "-X github.com/tektoncd/cli/pkg/cmd/version.namespace=tekton-pipelines1"
$ tkn version
Client version: dev
Pipeline version: unknown, pipeline controller may be installed in another namespace please use tkn version -n {namespace}
Triggers version: unknown, triggers controller may be installed in another namespace please use tkn version -n {namespace}
Dashboard version: unknown, dashboard controller may be installed in another namespace please use tkn version -n {namespace}

Specified correct namespace where controllers are running using flag

$ tkn version -n tekton-pipelines
Client version: dev
Pipeline version: v0.14.2
Triggers version: devel
Dashboard version: v0.9.0

Built tkn binary from master code
By default tkn version looks Deployment in tekton-pipeline or openshift-pipelines namespace

$ go build
$ tkn version
Client version: dev
Pipeline version: v0.14.2
Triggers version: devel
Dashboard version: v0.9.0

$ tkn version --help
Prints version information

Usage:
tkn version [flags]

Flags:
  -c, --check              check if a newer version is available
  -h, --help               help for version
  -n, --namespace string   namespace to check installed controller version

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 · 2020-07-31T11:31:57Z

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	78.1%	78.5%	0.3
pkg/version/version.go	91.1%	91.7%	0.6

tekton-robot · 2020-07-31T11:52:52Z

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	78.1%	78.5%	0.3
pkg/version/version.go	91.1%	91.7%	0.6

vdemeester

👍

savitaashture · 2020-07-31T14:02:48Z

/test pull-tekton-cli-integration-tests

piyush-garg · 2020-07-31T15:57:59Z

pkg/version/version.go

@@ -23,6 +23,9 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+// NOTE: use go build -ldflags "-X github.com/tektoncd/cli/pkg/version.defaultNamespace=tekton-pipelines"
+var defaultNamespace = "tekton-pipelines"


Is not better to put defaultNamespace value in flag itself

@piyush-garg wdym ? we need to have that variable "defined" so that we can overwrite it with the ldflags. Being here seems to make more sense to me than in the cmd part (with the flags).

Replied here https://github.com/tektoncd/cli/pull/1092/files#r464241241

chmouel · 2020-08-01T09:51:37Z

Not sure if that was discussed but can't we have a list of most common namespaces in defaultNamespace by default?

defaultNamespaces := []string{
   "tekton-pipeline",
   "openshift-pipeline",
   "otherinthefutureofpopularnamespacefortekton",
}

so an upstream tkn binary can work on openshift-pipelines by default (and vice versa) ?

we try the list in order to find the version in there?

savitaashture · 2020-08-03T03:33:21Z

pipeline

Hi @chmouel I don't remember exactly like we discussed to keep list of namespace but we can do that instead of one namespace tekton-pipeline we can keep 2 right now

defaultNamespaces := []string{
   "tekton-pipeline",
   "openshift-pipeline",
}

But i am not sure if we have someother namespace used by anyother vendor.

So shall i go ahead with those 2 namespace right now ?

piyush-garg · 2020-08-03T07:29:15Z

Makefile

 ifneq ($(SKIPLDFLAG),)
-	FLAGS := $(VERSIONLDFLAG) $(SKIPLDFLAG)
+	FLAGS := $(VERSIONLDFLAG) $(SKIPLDFLAG)$(NAMESPACEFLAG)


I think we need an if case for this also, there may be scenarios of only namespace flag provided

I am not sure i understand completely like in which scenario we need if condition

and in the current code also i see with and without if condition flags are same i mean without any condition flags are here and with condition flags are here and both are same correct me if i mistaken something

Makefile

piyush-garg · 2020-08-03T07:33:19Z

pkg/cmd/version/version.go

+	clientVersion = devVersion
+	// flag to skip check flag in version cmd
+	skipCheckFlag = "false"
+	namespace     string


@vdemeester I mean we can put the default value here, and this can be overridden with LDFLAG and also namespace flag in cmd. And we can further use this in all functions. This way we can remove the defaultNamespace flag from pkg cc @savitaashture

This can be single source for namespace then rather than two different for cmd and LDFLAG

@piyush-garg updated code to have same var for both cmd and LDFLAG but not for default because now default is an []string{} and as per my understanding LDFLAG doesn't take the [] .

piyush-garg · 2020-08-03T07:37:17Z

pipeline
Hi @chmouel I don't remember exactly like we discussed to keep list of namespace but we can do that instead of one namespace tekton-pipeline we can keep 2 right now
defaultNamespaces := []string{
   "tekton-pipeline",
   "openshift-pipeline",
}
But i am not sure if we have someother namespace used by anyother vendor.

So shall i go ahead with those 2 namespace right now ?

Nit: AFAIR, namespaces are

defaultNamespaces := []string{
   "tekton-pipelines",
   "openshift-pipelines",
}

chmouel · 2020-08-03T08:02:40Z

@savitaashture Sounds good to me ! (the list of @piyush-garg openshift-pipeline with s :))

vdemeester · 2020-08-03T08:19:51Z

defaultNamespaces := []string{
   "tekton-pipelines",
   "openshift-pipelines",
}

If we use that, how does ldflags work with an array of string ? 🤔

chmouel · 2020-08-03T08:27:29Z

@vdemeester If there is a ldflag that was specified it will :

Try the one from ldlfags
Try the defaultNamespaces

tekton-robot · 2020-08-04T18:39:12Z

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	78.1%	78.5%	0.3
pkg/version/version.go	91.1%	91.1%	-0.0

tekton-robot · 2020-09-14T11:44:46Z

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	79.4%	79.7%	0.3
pkg/version/version.go	91.5%	91.4%	-0.1

savitaashture · 2020-09-14T11:54:16Z

@chmouel @vdemeester @piyush-garg

Addressed all the comments and rebased please do have a look

Thank you

chmouel · 2020-09-14T12:19:24Z

@savitaashture i think the unit test needs to be fixed for 0_10?

savitaashture · 2020-09-15T12:11:23Z

/test pull-tekton-cli-integration-tests-0_10

savitaashture · 2020-09-15T12:26:32Z

@savitaashture i think the unit test needs to be fixed for 0_10?

@chmouel Done

vdemeester

/meow

tekton-robot · 2020-09-17T07:38:20Z

@vdemeester:

In response to this:

/meow

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.

piyush-garg · 2020-09-17T13:23:36Z

Makefile

 ifneq ($(SKIPLDFLAG),)
-	FLAGS := $(VERSIONLDFLAG) $(SKIPLDFLAG)
+	FLAGS := $(VERSIONLDFLAG) $(SKIPLDFLAG) $(NAMESPACELDFLAG)


we need to handle the cases here carefully, I think it will cause an issue when you will not have skip flag but have namespace flag

or

we need to handle namespace flag case

I have tested without specifying skip flag like below

GOOS=linux GOARCH=amd64 go build -mod=vendor -ldflags "-X github.com/tektoncd/cli/pkg/cmd/version.namespace=tekton-pipelines"

and it works without any issue

let me know if i miss understand the point which you have mentioned

You are not using makefile here :D

Updated makefile

tested with following scenario

NAMESPACE=tekton-pipelines SKIP_CHECK_FLAG=true RELEASE_VERSION=0.13 make amd64

NAMESPACE=tekton-pipelines make amd64

NAMESPACE=tekton-pipelines RELEASE_VERSION=0.13 make amd64

SKIP_CHECK_FLAG=true NAMESPACE=tekton-pipelines make amd64

piyush-garg · 2020-09-17T13:25:55Z

pkg/cmd/version/version.go

 				if pipelineVersion == "" {
-					pipelineVersion = "unknown"
+					pipelineVersion = "unknown, " +
+						"pipeline controller may be installed in another namespace please use tkn version -n {namespace}"


Do we need this, as we are planning to no show version in case of unknown

I see still it exist in the current code so in order to keep in sync with master added this change but if there is a plan to not to show unknown then i think we can remove while doing the PR related to removing unknown changes

WDYT?

tekton-robot · 2020-09-22T18:03:08Z

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	79.4%	79.7%	0.3
pkg/version/version.go	91.5%	91.4%	-0.1

savitaashture · 2020-09-22T18:26:47Z

/test pull-tekton-cli-integration-tests

tekton-robot · 2020-09-23T06:49:38Z

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	79.4%	79.7%	0.3
pkg/version/version.go	91.5%	91.4%	-0.1

savitaashture · 2020-09-23T07:23:54Z

/test pull-tekton-cli-integration-tests

tekton-robot · 2020-09-23T08:51:06Z

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	79.4%	79.7%	0.3
pkg/version/version.go	91.5%	91.4%	-0.1

pkg/cmd/version/version.go

piyush-garg

/lgtm

tekton-robot · 2020-09-24T09:14:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: piyush-garg, 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~~ [piyush-garg,vdemeester]

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

tekton-robot added the release-note label Jul 31, 2020

tekton-robot requested review from chmouel and piyush-garg Jul 31, 2020

tekton-robot added the size/L label Jul 31, 2020

savitaashture force-pushed the savitaashture:tknversion branch from 42ec180 to c563c76 Jul 31, 2020

vdemeester reviewed Jul 31, 2020

View changes

piyush-garg reviewed Jul 31, 2020

View changes

piyush-garg reviewed Aug 3, 2020

View changes

Makefile Outdated Show resolved Hide resolved

piyush-garg reviewed Aug 3, 2020

View changes

savitaashture force-pushed the savitaashture:tknversion branch from c563c76 to 8a4ac51 Aug 4, 2020

tekton-robot added the needs-rebase label Sep 2, 2020

savitaashture force-pushed the savitaashture:tknversion branch from 8a4ac51 to 84d3a68 Sep 14, 2020

tekton-robot removed the needs-rebase label Sep 14, 2020

vdemeester approved these changes Sep 17, 2020

View changes

tekton-robot added the approved label Sep 17, 2020

piyush-garg reviewed Sep 17, 2020

View changes

savitaashture force-pushed the savitaashture:tknversion branch from 84d3a68 to 60c5e65 Sep 22, 2020

savitaashture force-pushed the savitaashture:tknversion branch from 60c5e65 to 5325021 Sep 23, 2020

Modify tkn version to accept ldflag and namespace flag

Loading status checks…

eb625c7

savitaashture force-pushed the savitaashture:tknversion branch from 5325021 to eb625c7 Sep 23, 2020

piyush-garg reviewed Sep 23, 2020

View changes

pkg/cmd/version/version.go Show resolved Hide resolved

piyush-garg approved these changes Sep 24, 2020

View changes

tekton-robot assigned piyush-garg Sep 24, 2020

tekton-robot added the lgtm label Sep 24, 2020

Divyansh42 mentioned this pull request Sep 25, 2020

Modify tkn version output to hide components version if they are not installed #1157

Merged

4 of 4 tasks complete

This was referenced Sep 25, 2020

Fix deployment fetch issue for multiple namespaces #1186

Merged

Add role and rolebinding for openshift-pipelines namespace openshift/tektoncd-pipeline-operator#460

Merged

Nov	DEC	Jan
	22
2019	2020	2021

tektoncd / cli

Modify tkn version to accept namespace as ldflag and flag #1092

Modify tkn version to accept namespace as ldflag and flag #1092

Conversation

savitaashture commented Jul 31, 2020 • edited

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 31, 2020

tekton-robot commented Jul 31, 2020

vdemeester left a comment

savitaashture commented Jul 31, 2020

This comment has been minimized.

piyush-garg Jul 31, 2020 Contributor

This comment has been minimized.

vdemeester Jul 31, 2020 Member

This comment has been minimized.

piyush-garg Aug 3, 2020 Contributor

chmouel commented Aug 1, 2020

savitaashture commented Aug 3, 2020

This comment has been minimized.

piyush-garg Aug 3, 2020 Contributor

This comment has been minimized.

savitaashture Aug 4, 2020 Author Contributor

This comment has been minimized.

piyush-garg Aug 3, 2020 Contributor

This comment has been minimized.

piyush-garg Aug 3, 2020 Contributor

This comment has been minimized.

savitaashture Aug 4, 2020 • edited Author Contributor

piyush-garg commented Aug 3, 2020

chmouel commented Aug 3, 2020

vdemeester commented Aug 3, 2020

chmouel commented Aug 3, 2020

tekton-robot commented Aug 4, 2020

tekton-robot commented Sep 14, 2020

savitaashture commented Sep 14, 2020

chmouel commented Sep 14, 2020

savitaashture commented Sep 15, 2020

savitaashture commented Sep 15, 2020

vdemeester left a comment

tekton-robot commented Sep 17, 2020

This comment has been minimized.

piyush-garg Sep 17, 2020 • edited Contributor

This comment has been minimized.

savitaashture Sep 21, 2020 Author Contributor

This comment has been minimized.

piyush-garg Sep 22, 2020 Contributor

This comment has been minimized.

savitaashture Sep 22, 2020 Author Contributor

This comment has been minimized.

piyush-garg Sep 17, 2020 Contributor

This comment has been minimized.

savitaashture Sep 21, 2020 Author Contributor

tekton-robot commented Sep 22, 2020

savitaashture commented Sep 22, 2020

tekton-robot commented Sep 23, 2020

savitaashture commented Sep 23, 2020

tekton-robot commented Sep 23, 2020

piyush-garg left a comment

tekton-robot commented Sep 24, 2020

savitaashture commented Jul 31, 2020 •

edited

piyush-garg Jul 31, 2020
Contributor

vdemeester Jul 31, 2020
Member

piyush-garg Aug 3, 2020
Contributor

piyush-garg Aug 3, 2020
Contributor

savitaashture Aug 4, 2020
Author Contributor

piyush-garg Aug 3, 2020
Contributor

piyush-garg Aug 3, 2020
Contributor

savitaashture Aug 4, 2020 •

edited

Author Contributor

piyush-garg Sep 17, 2020 •

edited

Contributor

savitaashture Sep 21, 2020
Author Contributor

piyush-garg Sep 22, 2020
Contributor

savitaashture Sep 22, 2020
Author Contributor

piyush-garg Sep 17, 2020
Contributor

savitaashture Sep 21, 2020
Author Contributor