-
Notifications
You must be signed in to change notification settings - Fork 1.4k
🐛 Remove message from Runtime SDK FailureResponses #6933
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
🐛 Remove message from Runtime SDK FailureResponses #6933
Conversation
c73d506
to
9fe4687
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold
Just investigating if this has any impact on the e2e tests where we compare the blocking condition.
/unhold |
9fe4687
to
044d3c2
Compare
We might to eventually categorised the errors to provide a more meaningful message without triggering extra loops. /lgtm |
This is hard because these errors are currently returned by the user. The issue with Message as it is is that they're fully defined by the Runtime Extension. Even if we had fixed errors that could us e.g. errors.Cause() we couldn't be certain they'd be used without some additional validation (which would mean failing due to non-conforming error messages which seems like bad UX 😆) If you have any ideas on this though it would be great to hear them. |
044d3c2
to
87db68d
Compare
Signed-off-by: killianmuldoon <[email protected]>
87db68d
to
3545a08
Compare
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
/cherry-pick release-1.2
If in future we will reconsider this because user find it complex to investigate problems, we should make sure to document that error message should be idempotent
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
/cherry-pick release-1.2 |
@fabriziopandini: new pull request created: #6955 In response to this:
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. |
Signed-off-by: killianmuldoon [email protected]
Remove the Message returned from the Runtime extension from the error returned by CallExtension and Discovery. This message may be unique and could cause too many reconciliation loops as in #5945
Fixes #6921