-
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
string results that can be parsed as json should not fail #5814
Conversation
Skipping CI for Draft Pull Request. |
/test tekton-pipeline-unit-tests |
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.
/meow
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. |
/retest |
/approve |
@chuangw6 sorry I forgot to cc you. please take a look |
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.
Thanks @Yongxuanzhang !!
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.
Thank you for this.
A small nit but nothing blocking.
Would you mind updating the release notes to include what changes as opposed to what should be.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, chitrangpatel, 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 |
Thanks for fixing this. This is a blocker for us to upgrade to the latest release. I wonder whether it makes sense to add notes to the released version (since the validation was added) with additional description like Known Issues.
|
Sure. @zhouhaibing089 What did you mean by adding notes to the released version? |
b739cf2
to
558281b
Compare
The following is the coverage report on the affected files.
|
@Yongxuanzhang: We tried to upgrade to v0.40.2 (the last version which still has support to v1alpha1), and this issue occured (we have |
Thanks! This is a great suggestion. But I'm not sure if there's such a section. How about adding it to the issue as well? |
@jerop Can we merge this into v0.43 btw? Thanks! |
This commit fixes tektoncd#5803. In tep75&76 we introduced object results. The object results is inferred from the emitted message, and if the message is of json format, then the result type could be object or array. We later validate the type with the type we defined in taskspec. So if the taskpec type is string it will fail validation. Based on the discussions in tektoncd#5803, if the type in taskpec is string, we should keep the results as is and don't convert it to object or array.
558281b
to
a477cc2
Compare
The following is the coverage report on the affected files.
|
/lgtm |
Changes
This commit fixes #5803. In tep75&76 we introduced object results. The object results is inferred from the emitted message, and if the message is of json format, then the result type could be object or array. We later validate the type with the type we defined in taskspec. So if the taskpec type is string it will fail validation. Based on the discussions in #5803, if the type in taskpec is string, we should keep the results as is and don't convert it to object or array.
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes