Promote Graceful termination to stable #4668
Conversation
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
thanks for helping out here @vinamra28, just took a quick glance - will wait for it to be ready for review to take a closer look
/assign
closes #4611
60c0049 to
802e01f
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
802e01f to
d07d817
Compare
|
The following is the coverage report on the affected files.
|
d07d817 to
e7d57b6
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
thanks again @vinamra28!
please remove graceful termination from the alpha features in the install docs - https://github.com/tektoncd/pipeline/blob/f2da9a877f295a6cb865d8dee7b52872089c061d/docs/install.md#alpha-features
|
@vinamra28 - we should wait for the fixes related to graceful stopping as described in #4651 (comment) to go in first |
|
The following is the coverage report on the affected files.
|
a604cdf to
ba5a04b
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
ac9eb47 to
7c35c52
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
7c35c52 to
4666c1d
Compare
|
@vdemeester @jerop @pritidesai @imjasonh I have updated the PR based on above feedbacks. PTAL |
|
/retest |
|
The following is the coverage report on the affected files.
|
| @@ -29,7 +29,7 @@ import ( | |||
|
|
|||
| "github.com/tektoncd/pipeline/test/parse" | |||
|
|
|||
| "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" | |||
| "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" | |||
There was a problem hiding this comment.
looks like this test is meant for v1alpha1, maybe keep this as is?
There was a problem hiding this comment.
yeah test is meant for v1alpha1 but the status being used here were from v1beta1. Once PipelineRunCancelled is removed, we anyhow will have to use v1beta1 status. Moreover v1alpha1 gets converted to v1beta1 during runtime
| PipelineRunSpecStatusStoppedRunFinally, | ||
| PipelineRunSpecStatusPending), "status") | ||
| } | ||
| return apis.ErrInvalidValue(fmt.Sprintf("%s should be %s or %s", status, |
There was a problem hiding this comment.
It seems like removing this will mean that the deprecated value will no longer work, and it seems like from previous PR comments that we want to avoid removing this value until the new value is promoted to beta.
There was a problem hiding this comment.
@lbernick it's just the webhook validation which was checking for the fields that were behind feature flag.
There was a problem hiding this comment.
@lbernick it's just the error message. We could still include PipelineRunSpecStatusCancelledDeprecated but as it is deprecated, I don't really think it would make sense.
With the addition of graceful termination in v0.25.x release, `PipelineRunCancelled` was deprecated. It's been 8 months and no such issues so far, we can promote these to beta. In this patch, unguarding the graceful termination from alpha feature flag and modifying the tests based on this. Signed-off-by: vinamra28 <jvinamra776@gmail.com>
4666c1d to
83807c4
Compare
|
The following is the coverage report on the affected files.
|
| PipelineRunSpecStatusStoppedRunFinally, | ||
| PipelineRunSpecStatusPending), "status") | ||
| } | ||
| return apis.ErrInvalidValue(fmt.Sprintf("%s should be %s or %s", status, |
There was a problem hiding this comment.
@lbernick it's just the error message. We could still include PipelineRunSpecStatusCancelledDeprecated but as it is deprecated, I don't really think it would make sense.
|
[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 |
@vdemeester are you referring to this file? it's already there |
|
/lgtm |
Changes
With the addition of graceful termination in v0.25.x release,
PipelineRunCancelledwas deprecated. It's been 8 months and no suchissues so far, we can promote these to beta.
In this patch, unguarding the graceful termination from alpha feature
flag and modifying the tests based on this.
closes #4611
Signed-off-by: vinamra28 jvinamra776@gmail.com
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes