-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add granular termination reason in container termination message #7390
Conversation
Hi @renzodavid9. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
The following is the coverage report on the affected files.
|
ef3dff7
to
0ed5348
Compare
0ed5348
to
834b895
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
834b895
to
2a3a73b
Compare
The following is the coverage report on the affected files.
|
2a3a73b
to
dd3b700
Compare
The following is the coverage report on the affected files.
|
/ok-to-test |
The following is the coverage report on the affected files.
|
17688e0
to
7f6023a
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
7f6023a
to
13ecf81
Compare
cc @chitrangpatel for the context of #7223 |
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.
|
/hold Would changing the Reason to a different string be something that is considered a breaking change according to our API compatibility policy? #7223 (comment) shows the changes to the Looking for feedback before we merge this. Wondering how this might affect users of this field (maybe @tektoncd/dashboard-maintainers ) cc @tektoncd/core-maintainers |
/approve |
[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:
Approvers can indicate their approval by writing |
From my understanding, the change shall not fall under the categorization of a breaking change. Given its context, it seems more of a "bug" fix rather than breaking our users given that we were previously covering the actual termination reasons. |
Related with tektoncd#7223. To report specific Steps termination reasons we need to know why its continer finished; we use the termination message to store a new "state" with this information. We evaluated changing the container `reason` directly, but looks like k8s doesn't allow this. Co-authored-by: JeromeJu <[email protected]> Co-authored-by: Chitrang Patel <[email protected]>
f682841
to
2b5e3f3
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
This will affect the Dashboard, and likely any other client, which relies on the current I'm not sure I agree with the original categorisation of #7223 as a bug, it seems more like an enhancement to differentiate between these cases with more granular |
I agree with @AlanGreene there. It's gonna affect any clients, so we have to take any change there very carefully, and ideally, in a non breaking way. |
Sound good! I'm glad that I put a hold on it 😅. Let's discuss this more at the API WG call on Monday (I believe there is agenda item for this already). |
Fixes #7223.
To report specific Steps termination reasons we need to know why its continer finished; we use the termination message to store a new "state" with this information. We evaluated changing the container
reason
directly, but looks like k8s doesn't allow this./kind feature
Changes
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes