Use kubectl create instead of apply #1398
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/XS
tekton-robot
added
size/S
| @@ -33,4 +33,4 @@ spec: | |||
| inputs: | |||
| params: | |||
| - name: SCNAME | |||
There was a problem hiding this comment.
I think we need to have the type here… This feels weird to me but looking at the error…
I1009 13:16:09.670] Error from server (InternalError): error when creating "STDIN": Internal error occurred: admission webhook "webhook.tekton.dev" denied the request: mutation failed: invalid value: : taskspec.inputs.params.SCNAME.type
There was a problem hiding this comment.
Nice catch. It may be because it's in a taskSpec embedded in a TaskRun.
I fixed this and all other occurrences.
tekton-robot
added
size/M
afrittoli
force-pushed
the
issue/1397
branch
2 times, most recently
from
d2ebe07 to
6f4efc4
Compare
| @@ -16,7 +16,7 @@ spec: | |||
| apiVersion: rbac.authorization.k8s.io/v1 | |||
| kind: ClusterRoleBinding | |||
| metadata: | |||
| name: default-cluster-admin | |||
There was a problem hiding this comment.
I was going to say we probably shouldn't recommend this in our examples, since it will result in multiple identical role bindings between default<->cluster-admin, but there probably shouldn't even be one default<->cluster-admin role binding in our examples, since that's a pretty bad idea from a security standpoint. 😬
There was a problem hiding this comment.
Totally +1 on the spirit of the change, but I think it uncovers some things we'll want to clean up. Some of these PipelineResources should just be embedded in the Pipeline that uses them, which would make a lot of this cleaner.
Good point. I'm trying to provide a quick solution to this because right now we are not running the taskrun examples anymore, which is pretty bad.
Embedding PipelineResources would be nice, but I'd rather do it on the next iteration.
Another approach to solve the problem could be to create a namespace for every example, which - with the exception of clustertasks - would render all resource name unique.
tekton-robot
added
the
do-not-merge/hold
tekton-robot
added
lgtm
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
|
[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:
Approvers can indicate their approval by writing |
|
/test pull-tekton-pipeline-integration-tests |
When running YAML E2E tests, use kubectl create instead of apply, so that example can use generateName, which is a best practice for run objects (TaskRun/PipelineRun). Using "create" also exposed name conflicts for several resources all named the same. Luckily the content of those secret was the same, so we had no race condition in the past, but it's best to avoid side effects between tests as much as possible and keep resource names uniques across examples. With this change we start running taskrun tests again, and it turns out the gitlab test is broken because of a broken gitlab URL. Introduce a subfolder for examples that are valid but that we don't want to run in CI because they need some access to a resource which is not available. The Gitlab test has been removed before this was merged, but keeping the no-ci folder anyways as it may be useful for examples that we have no easy way to run in CI. Fixes tektoncd#1397
|
/lgtm |
When running YAML E2E tests, use kubectl create instead of apply,
so that example can use generateName, which is a best practice
for run objects (TaskRun/PipelineRun).
Fixes #1397
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe 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