Skip to content
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

Clean up pipelinerun resolution code #6628

Closed
lbernick opened this issue May 5, 2023 · 2 comments · Fixed by #6801
Closed

Clean up pipelinerun resolution code #6628

lbernick opened this issue May 5, 2023 · 2 comments · Fixed by #6801
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@lbernick
Copy link
Member

lbernick commented May 5, 2023

Pipelinerun resolution code (e.g. https:/tektoncd/pipeline/blob/main/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go) is getting super hard to understand, making it challenging to evaluate code that modifies it (e.g. #6603 (comment)).

Some specific changes that would go a long way for readability:

  • Adding docstrings for functions in the resources package, especially pipelinerunresolution.go
  • Removing redundant information from ResolvedPipelineTask. For example, this struct contains "TaskRun", "TaskRuns", "TaskRunName", and "TaskRunNames". It's hard to know which of these fields code should use.
  • Renaming references to "resources". I suspect this originally referred to PipelineResources, and expanded over time to refer to resolving things like tasks, params, workspaces etc etc.
  • Try to avoid overloading the word "resolve". This is currently used to refer to fetching remote tasks, getting all the taskruns from a pipeline task, getting results from previous tasks, etc.

Another aspect of ResolvedPipelineTask that's confusing is that it's hard to know at what point in the code you can expect referenced TaskRuns and CustomRuns to be non-nil. For example, if ResolvedPipelineTask.TaskRun is nil, does that mean there is no taskrun associated with the pipeline task yet, or just that we haven't yet fetched it from the cluster? (Or that it's a matrixed pipeline task, and uses TaskRuns instead?)

cc @EmmaMunley

@lbernick lbernick added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels May 5, 2023
@EmmaMunley
Copy link
Contributor

/assign

EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 11, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 11, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 11, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 11, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 11, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 11, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 12, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 15, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 15, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 15, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 16, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 16, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 16, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 16, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 16, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 17, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 17, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 17, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 18, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 18, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 19, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
EmmaMunley added a commit to EmmaMunley/pipeline that referenced this issue May 22, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.
tekton-robot pushed a commit that referenced this issue May 22, 2023
Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in  ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames,  ObjectRuns, ObjectRunNames can be used  regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue #6628.
@lbernick
Copy link
Member Author

between #6643 and #6649 I think we're good to close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants