tektoncd / pipeline

A cloud-native Pipeline resource.
https://tekton.dev
Apache License 2.0
8.35k stars 1.76k forks source link

Cleanup resolved object before validating through dry-run #8051

Closed vdemeester closed 2 weeks ago

vdemeester commented 2 weeks ago

Changes

This ensure that we are not going to fail during validation with dry-run. An example of such a failure would be the following scenario.

/kind bug

Signed-off-by: Vincent Demeester vdemeest@redhat.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Release Notes

Cleanup resolved object before attempting to validate it through api dry-run call
tekton-robot commented 2 weeks ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

tekton-robot commented 2 weeks ago

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go 94.1% 94.3% 0.2
pkg/reconciler/taskrun/resources/taskref.go 94.0% 94.2% 0.2
vdemeester commented 2 weeks ago

cc @JeromeJu @chitrangpatel This probably need some tests (unit at least), a release note and some refactoring, but does the approach sounds alright ? Also, we can list other fields we may want to clean.

tekton-robot commented 2 weeks ago

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go 94.1% 94.3% 0.2
pkg/reconciler/taskrun/resources/taskref.go 94.0% 94.2% 0.2
tekton-robot commented 2 weeks ago

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go 94.1% 94.3% 0.2
pkg/reconciler/taskrun/resources/taskref.go 94.0% 94.2% 0.2
tekton-robot commented 2 weeks ago

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

File Old Coverage New Coverage Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go 94.1% 94.3% 0.2
pkg/reconciler/taskrun/resources/taskref.go 94.0% 94.2% 0.2
tekton-robot commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chitrangpatel

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](https://github.com/tektoncd/pipeline/blob/main/OWNERS)~~ [chitrangpatel] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
vdemeester commented 2 weeks ago

Right now, we only use dryRun to Validate. If users have mutating admission webhooks on the cluster then when we do the dryRun and validate, the mutations will happen before the validation. However, we ignore the mutated object and only continue with the original runtime Object even though we have validated the mutated one. In practice, when we do the dryrun, we should return the mutated object in addition to the validation error so that we use the mutated object in the underlying spec.

Ah that's a good point, we should probably track this into an issue.

vdemeester commented 2 weeks ago

/cherry-pick v0.59.x

tekton-robot commented 2 weeks ago

@vdemeester: once the present PR merges, I will cherry-pick it on top of v0.59.x in a new PR and assign it to you.

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175092971): >/cherry-pick v0.59.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
vdemeester commented 2 weeks ago

/cherry-pick v0.56.x

tekton-robot commented 2 weeks ago

@vdemeester: once the present PR merges, I will cherry-pick it on top of v0.56.x in a new PR and assign it to you.

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175093251): >/cherry-pick v0.56.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
vdemeester commented 2 weeks ago

/cherry-pick v0.53.x

tekton-robot commented 2 weeks ago

@vdemeester: once the present PR merges, I will cherry-pick it on top of v0.53.x in a new PR and assign it to you.

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175093509): >/cherry-pick v0.53.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
vdemeester commented 2 weeks ago

/cherry-pick v0.50.x

tekton-robot commented 2 weeks ago

@vdemeester: once the present PR merges, I will cherry-pick it on top of v0.50.x in a new PR and assign it to you.

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175093825): >/cherry-pick v0.50.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
savitaashture commented 2 weeks ago

/lgtm

tekton-robot commented 2 weeks ago

@savitaashture: changing LGTM is restricted to collaborators

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175097199): >/lgtm Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
jkandasa commented 2 weeks ago

/lgtm

tekton-robot commented 2 weeks ago

@jkandasa: changing LGTM is restricted to collaborators

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175098292): >/lgtm Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
piyush-garg commented 2 weeks ago

/lgtm

tekton-robot commented 2 weeks ago

@vdemeester: cannot checkout v0.59.x: error checking out v0.59.x: exit status 1. output: error: pathspec 'v0.59.x' did not match any file(s) known to git

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175092971): >/cherry-pick v0.59.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
tekton-robot commented 2 weeks ago

@vdemeester: cannot checkout v0.56.x: error checking out v0.56.x: exit status 1. output: error: pathspec 'v0.56.x' did not match any file(s) known to git

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175093251): >/cherry-pick v0.56.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
tekton-robot commented 2 weeks ago

@vdemeester: cannot checkout v0.53.x: error checking out v0.53.x: exit status 1. output: error: pathspec 'v0.53.x' did not match any file(s) known to git

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175093509): >/cherry-pick v0.53.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
tekton-robot commented 2 weeks ago

@vdemeester: cannot checkout v0.50.x: error checking out v0.50.x: exit status 1. output: error: pathspec 'v0.50.x' did not match any file(s) known to git

In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175093825): >/cherry-pick v0.50.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
vdemeester commented 2 weeks ago

/cherry-pick release-v0.59.x

tekton-robot commented 2 weeks ago

@vdemeester: #8051 failed to apply on top of branch "release-v0.59.x":

Applying: Cleanup resolved object before validating through dry-run
Using index info to reconstruct a base tree...
M   pkg/reconciler/pipelinerun/resources/pipelineref.go
M   pkg/reconciler/taskrun/resources/taskref.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/reconciler/taskrun/resources/taskref.go
CONFLICT (content): Merge conflict in pkg/reconciler/taskrun/resources/taskref.go
Auto-merging pkg/reconciler/pipelinerun/resources/pipelineref.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Cleanup resolved object before validating through dry-run
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
In response to [this](https://github.com/tektoncd/pipeline/pull/8051#issuecomment-2175509300): >/cherry-pick release-v0.59.x Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
vdemeester commented 2 weeks ago

Arf… I'll have to manually cherry-pick..