-
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
Use SchemeGroupVersion for tekton objects in cluster resolver #5705
Conversation
The following is the coverage report on the affected files.
|
/retest |
Prior, the cluster resolver sets the APIVersion of the retrieved tekton object to be a fixed string "tekton.dev/v1beta1". Now, we changed this to `v1beta1.SchemeGroupVersion.String()` for easier transition to v1 once v1 is released. Signed-off-by: Chuang Wang <[email protected]>
c2f54e5
to
4bafd12
Compare
The following is the coverage report on the affected files.
|
@@ -101,6 +101,7 @@ func (r *Resolver) Resolve(ctx context.Context, origParams []pipelinev1beta1.Par | |||
} | |||
|
|||
var data []byte | |||
groupVersion := pipelinev1beta1.SchemeGroupVersion.String() |
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 Chuang! I was wondering if there is any way we can link this to the storage version?
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.
What could happen if the version is not correctly specified?
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.
Sorry for the delay @JeromeJu .
I was wondering if there is any way we can link this to the storage version?
Not sure how we can get the storage version. I think pipelinev1beta1.SchemeGroupVersion.String()
might be sufficient here as andrew suggested here. Once we switched to v1, we need to change the imported pkg and this version will be changed accordingly I think.
What could happen if the version is not correctly specified?
That's a good question. I just played around this. If the typemeta i.e. the version or kind is not set correctly, the resolution will still succeed, but the code of converting the retrieved content to the runtime object will fail. And it will complain "no kind "Task" is registered for version "v1" in scheme ...", which makes sense because we don't serve the v1 CRD in the cluster.
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 for the findings Chuang, and yeah I think that is very reasonable.
/approve |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abayer, 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 |
/retest |
Changes
/kind cleanup
Prior, the cluster resolver sets the APIVersion of the retrieved tekton object to be a fixed string "tekton.dev/v1beta1".
Now, we changed this to
v1beta1.SchemeGroupVersion.String()
for easier transition to v1 once v1 is released.Signed-off-by: Chuang Wang [email protected]
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