Error sweep: Move TaskRun Reasons in pkg/pod to pkg/apis #7406
Conversation
cb89738 to
9ca47aa
Compare
9ca47aa to
d9794b8
Compare
This commit removes the unused pod.ReasonCouldntGetTask. ReasonCouldntGetTask has been moved from reconciler to pod/status pkg via tektoncd#1627. The reference to it has been removed since tektoncd#6295. /kind cleanup
2510528 to
19f627f
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
/assign |
| @@ -349,7 +352,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, | |||
| if resources.IsErrTransient(err) { | |||
| return nil, nil, err | |||
| } | |||
| tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) | |||
| tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedResolution, err) | |||
There was a problem hiding this comment.
Should we avoid using podconvert.xxx error reason when it is just a alias to v1.xxx error reason across the file (for example line 346). My understanding the alisa is just for backward compatability and should no longer be used in our codebase
There was a problem hiding this comment.
Yes that is correct. Those reasons were once living in pkg/reconciler and moved to pkg/pod. But they are actually tied with api since it is part of the apis.condition reason for TaskRunStatus. I tried to call out this at [#L41](// Aliased for backwards compatibility; do not add additional TaskRun reasons here).
There was a problem hiding this comment.
I've also updated all reference to podconvert.*reasons to use v1.*TaskRunReasons instead.
| // ReasonFailedResolution indicated that the reason for failure status is | ||
| // that references within the TaskRun could not be resolved | ||
| ReasonFailedResolution = "TaskRunResolutionFailed" | ||
|
|
||
| ReasonFailedResolution = v1.TaskRunReasonFailedResolution.String() |
There was a problem hiding this comment.
why do we need a .String() here?
There was a problem hiding this comment.
The reasons previously were just strings while the v1.TaskRunReasons* are of type TaskRunReason.
There was a problem hiding this comment.
Yeah, it looks like only ReasonFailedResolution is attached with .String() before. Now it is consistent. Thanks.
Prior to this commit, strings that were set as Reasons for the TaskRun status were split between pkg/apis and pkg/reconciler/taskrun. This commit moves all TaskRun related reasons to pkg/apis and adds aliases for backwards compatibility. This adds consistency, correctly signals to clients that all Reasons are part of the API, and helps avoid circular imports. It also renames ReasonPending to ReasonPodPending. /kind cleanup fixes: tektoncd#7397
19f627f to
ff881a5
Compare
|
/lgtm |
Changes
Prior to this commit, strings that were set as Reasons for the TaskRun status
were split between pkg/apis and pkg/reconciler/taskrun. This commit moves all TaskRun related
reasons to pkg/apis and adds aliases for backwards compatibility. This adds consistency,
correctly signals to clients that all Reasons are part of the API, and helps avoid circular imports.
It also renames ReasonPending to ReasonPodPending.
/kind cleanup
fixes: #7397
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