Remove the custom "name" parameter from cluster resources. #1474

dlorenc · 2019-10-25T15:23:27Z

Changes

This is redundant with the Name that Tasks already declare on Resources.
The name is used both to name the cluster in the kubeconfig and to set the
path for the kubeconfig. Using a name on the resource itself means Tasks can't
know where to expect this file.

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)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

The 'name' parameter to the cluster resource is now deprecated. Please use the standard name parameter on the Task Resource binding instead.

afrittoli

I think we should keep the name param - see also #492 and #431

pkg/apis/pipeline/v1alpha1/cluster_resource.go

afrittoli · 2019-10-25T15:30:56Z

pkg/apis/pipeline/v1alpha1/cluster_resource.go

@@ -62,11 +62,12 @@ func NewClusterResource(kubeconfigWriterImage string, r *PipelineResource) (*Clu
 	clusterResource := ClusterResource{
 		Type:                  r.Spec.Type,
 		KubeconfigWriterImage: kubeconfigWriterImage,
+		Name:                  r.Name,


If I understand this correctly, this uses the resource name as cluster name. Unfortunately this is not OK - which is the reason the name parameter was added - because the resource name must be a DNS valid name, while the cluster name may be not a valid DNS one e.g. my_cluster is a valid cluster name, but not a valid resource name.

Ah thanks for the explanation. Can you explain why the cluster name matters in your case?

Leaving the name parameter would be fine I think as long as we write to the /workspace/resource-name path instead of the /workspace/cluster-name-param path.

I can't actually see the use case for cluster name though - it's only used inside the .kubeconfig AFAICT, and can be literally anything.

So, indeed it doesn't seem to matter at all, sorry about my bad review :(

afrittoli · 2019-11-26T13:26:05Z

I tested if the cluster name is used at all when connecting to a OpenShift cluster on IBM Cloud - and it is not - see the cluster name set to foobar

$ tkn resource describe andreaf-tekton
Name:                    andreaf-tekton
Namespace:               default
PipelineResource Type:   cluster

Params
NAME       VALUE
name       foobar
url        [xyz]
username   release-testing
cadata

Secret Params
FIELDNAME   SECRETNAME
token       release-testing-token-txdjz

$ tkn task start test-cluster -i k8s-cluster=andreaf-tekton
Taskrun started: test-cluster-run-b2bn8
Showing logs...
Task still running ...

[deploy-tekton-project] + tkn resource list
[deploy-tekton-project] NAME   TYPE   DETAILS
[deploy-tekton-project] test   git    url: https://github.com/tektoncd/pipeline

The name of the cluster is actually not used

afrittoli

Thanks for this.
Since this is done in a backward compatible way, it should be fine to merge it in v0.9.0

/lgtm

bobcatfish · 2019-11-26T15:13:51Z

/approve

tekton-robot · 2019-11-26T15:13:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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

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

bobcatfish · 2019-11-26T15:49:35Z

testing.tRunner
==================
WARNING: DATA RACE
Write at 0x00c0005c2fa0 by goroutine 57:
  github.com/tektoncd/pipeline/pkg/reconciler/taskrun.(*Reconciler).updateStatus()
      /go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/taskrun.go:428 +0x33e
  github.com/tektoncd/pipeline/pkg/reconciler/taskrun.(*Reconciler).updateStatusLabelsAndAnnotations()
      /go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/taskrun.go:172 +0x550
  github.com/tektoncd/pipeline/pkg/reconciler/taskrun.(*Reconciler).Reconcile()
      /go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/taskrun.go:161 +0xcfe
  github.com/tektoncd/pipeline/pkg/reconciler/taskrun.TestReconcile_ExplicitDefaultSA.func1()
      /go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/taskrun_test.go:385 +0x682
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199

🏎️

This is redundant with the Name that Tasks already declare on Resources. The name is used both to name the cluster in the kubeconfig and to set the path for the kubeconfig. Using a name on the resource itself means Tasks can't know where to expect this file.

bobcatfish · 2019-11-26T16:08:10Z

/lgtm

This was deprecated in tektoncd#1474 which was released in 0.9.0. It can safely be deleted in the next release.

This was deprecated in #1474 which was released in 0.9.0. It can safely be deleted in the next release.

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Oct 25, 2019

tekton-robot requested review from dibyom and imjasonh October 25, 2019 15:23

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 25, 2019

dlorenc force-pushed the morecluster branch 2 times, most recently from d52ebb8 to 9820b08 Compare October 25, 2019 15:32

afrittoli previously requested changes Oct 25, 2019

View reviewed changes

afrittoli reviewed Nov 26, 2019

View reviewed changes

tekton-robot assigned afrittoli Nov 26, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2019

dlorenc force-pushed the morecluster branch from 9820b08 to e1bfcc1 Compare November 26, 2019 15:54

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019

tekton-robot assigned bobcatfish Nov 26, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019

tekton-robot merged commit 124f101 into tektoncd:master Nov 26, 2019

dlorenc added a commit to dlorenc/build-pipeline that referenced this pull request Dec 3, 2019

Remove support for the name parameter to cluster resources.

d3d35e7

This was deprecated in tektoncd#1474 which was released in 0.9.0. It can safely be deleted in the next release.

dlorenc mentioned this pull request Dec 3, 2019

Remove support for the name parameter to cluster resources. #1672

Merged

3 tasks

dlorenc added a commit to dlorenc/build-pipeline that referenced this pull request Dec 3, 2019

Remove support for the name parameter to cluster resources.

5b8a7b3

This was deprecated in tektoncd#1474 which was released in 0.9.0. It can safely be deleted in the next release.

tekton-robot pushed a commit that referenced this pull request Dec 4, 2019

Remove support for the name parameter to cluster resources.

ad6045d

This was deprecated in #1474 which was released in 0.9.0. It can safely be deleted in the next release.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the custom "name" parameter from cluster resources. #1474

Remove the custom "name" parameter from cluster resources. #1474

dlorenc commented Oct 25, 2019

afrittoli left a comment

afrittoli Oct 25, 2019

dlorenc Oct 25, 2019

afrittoli Nov 26, 2019

afrittoli commented Nov 26, 2019

afrittoli left a comment

bobcatfish commented Nov 26, 2019

tekton-robot commented Nov 26, 2019

bobcatfish commented Nov 26, 2019

bobcatfish commented Nov 26, 2019

Remove the custom "name" parameter from cluster resources. #1474

Remove the custom "name" parameter from cluster resources. #1474

Conversation

dlorenc commented Oct 25, 2019

Changes

Submitter Checklist

Reviewer Notes

Release Notes

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Oct 25, 2019

Choose a reason for hiding this comment

dlorenc Oct 25, 2019

Choose a reason for hiding this comment

afrittoli Nov 26, 2019

Choose a reason for hiding this comment

afrittoli commented Nov 26, 2019

afrittoli left a comment

Choose a reason for hiding this comment

bobcatfish commented Nov 26, 2019

tekton-robot commented Nov 26, 2019

bobcatfish commented Nov 26, 2019

bobcatfish commented Nov 26, 2019