-
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
clarify in docs to not use apiVersion for taskRef for non-customtask #6704
Conversation
Skipping CI for Draft Pull Request. |
@@ -224,7 +224,7 @@ type PipelineTask struct { | |||
func (et *EmbeddedTask) IsCustomTask() bool { | |||
// Note that if `apiVersion` is set to `"tekton.dev/v1beta1"` and `kind` is set to `"Task"`, | |||
// the reference will be considered a Custom Task - https://github.com/tektoncd/pipeline/issues/6457 | |||
return et != nil && et.APIVersion != "" && et.Kind != "" | |||
return et != nil && et.APIVersion != "" && et.Kind != "" && et.Kind != string(ClusterTaskKind) |
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.
I wonder if checking (et.APIVersion!="v1beta1"&&et.Kind != string(ClusterTaskKind)) would be better?
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.
ah don't think we should handle ClusterTask
differently from Task
-- maybe what we need is better documentation?
/assign @jerop |
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.
Can you please add some pipelinerun reconciler unit tests for this change? It might be beneficial to have e2e tests as well but we can start with reconciler 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.
maybe what we need is better documentation that if apiversion
and kind
are set, we will create a customrun
-- #6457
@@ -224,7 +224,7 @@ type PipelineTask struct { | |||
func (et *EmbeddedTask) IsCustomTask() bool { | |||
// Note that if `apiVersion` is set to `"tekton.dev/v1beta1"` and `kind` is set to `"Task"`, | |||
// the reference will be considered a Custom Task - https://github.com/tektoncd/pipeline/issues/6457 | |||
return et != nil && et.APIVersion != "" && et.Kind != "" | |||
return et != nil && et.APIVersion != "" && et.Kind != "" && et.Kind != string(ClusterTaskKind) |
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.
ah don't think we should handle ClusterTask
differently from Task
-- maybe what we need is better documentation?
From the linked issue, it sounds like this used to create TaskRuns instead of CustomRuns, and this was a regression when updating to v0.44. If you can't specify "ClusterTask" as the "kind" in a TaskRef, how would you specify a cluster task? |
To clarify, you can create a "ClusterTask" with "kind" in TaskRef -- you just can't put APIVersion as well because that creates a CustomRun -- the user removed APIVersion and this resolved their issue |
Yes, I wonder if we need to fix it in code so users can still use apiversion+kind for cluster tasks? |
I think we should document instead of changing the implementation because some users are relying on apiversion and kind being set to create customruns -- #6457 If we make an exception for "ClusterTask", would we do the same for "Task" and "Pipeline"? What about the Custom Task controllers that depend on the existing behavior, such as Pipeline in Pipeline Custom Task? |
@Yongxuanzhang please update the commit and PR with more context, including #6459, #6505, #6457 |
What confused me is the "apiversion", if it is only used for custom tasks, and we don't use it for cluster task, tasks (since the storage version in the cluster is fixed, it is not possible to refer v1 tasks and v1beta1 tasks in the same cluster), we do need to clarify it to users (docs or even rename it) I lean to update the docs to clarify it now, since there's no need to use "apiversion"+"kind" for cluster tasks and tasks, pipelines. Though we do support it in previous versions when the custom task is not enabled by default and we should have called this out to users when we introduced this change. |
Ah sorry, I missed that. However, it does still sound like the user experienced different behavior for existing CRDs when upgrading. @Yongxuanzhang I'm curious why you say this might have started happening when we enabled custom tasks by default? I agree with Yongxuan that if we only use APIVersion for custom tasks, it would be nice to rename it in some way. (We can't remove it but can add a new field and document that instead.) I still find it confusing that "kind=Task" or ClusterTask, Pipeline, etc creates a custom task when apiversion is used :( but I'm ok with improving docs for now |
Previously, if custom task is not enabled, users can use apiversion+task/clustertask in taskref to refer tasks, but after we enabled it by default, the taskref will be considered as a custom task and creates customrun instead of taskrun |
@Yongxuanzhang: The label(s) 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. |
@Yongxuanzhang: The label(s) 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. |
/remove-kind bug |
Yongxuan can you check if we have any examples in the docs that use APIVersion for TaskRefs that aren't custom tasks? |
I cannot find any examples in docs, but found one in unit tests, just removed it |
Thanks, this lgtm, just two quick suggestions:
|
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.
I think you may need to update the PR description
This commit closes tektoncd#6682. When apiversion and kind are both set for task, it is also considered to be a custom task. If users want to refer to tasks or cluster tasks, apiVersion shouldn't be set. This commit clarify this in the docs. Signed-off-by: Yongxuan Zhang [email protected]
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: jerop, lbernick 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 |
Changes
This commit closes #6682. When apiversion and kind are both set for
task, it is also considered to be a custom task. If users want to refer
to tasks or cluster tasks, apiVersion shouldn't be set. This commit
clarify this in the docs.
/kind bug
Signed-off-by: Yongxuan Zhang [email protected]
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