Redesign PAC Resolver #1762

piyush-garg · 2024-09-10T23:35:58Z

This will redesign resolver to work with
multiple edge case scenarios.

Previously pac resolver was filtering
pipelinerun based on annotations but it was
resolving all pipelines in .tekton dir
leading to resolving unnecessary pipelines
and other issue was it was storing task
based on task name, instead of annotation name
and version, so different version of task
were not used across pipelineruns in .tekton dir

Now with new design we are first filtering
pipelinerun based on annotations, and then
processing all pipelineruns one by one
and only resolving pipeline related to these
pipelineruns. Also we are now maintaining map
of tasks with name and version at event level
to not re fetch the task and now inline spec
replacement in pipelinerun is done one by one
so respective task as mentioned in annotation
with name and version is used

Also before filtering the pipelineruns, we should
make sure that no two pipelineruns exists with
same name in .tekton dir

Added tests for the three scenarios

Submitter Checklist

📝 Please ensure your commit message is clear and informative. For guidance on crafting effective commit messages, refer to the How to write a git commit message guide. We prefer the commit message to be included in the PR body itself rather than a link to an external website (ie: Jira ticket).
♽ Before submitting a PR, run make test lint to avoid unnecessary CI processing. For an even more efficient workflow, consider installing pre-commit and running pre-commit install in the root of this repository.
✨ We use linters to maintain clean and consistent code. Please ensure you've run make lint before submitting a PR. Some linters offer a --fix mode, which can be executed with the command make fix-linters (ensure markdownlint and golangci-lint tools are installed first).
📖 If you're introducing a user-facing feature or changing existing behavior, please ensure it's properly documented.
🧪 While 100% coverage isn't a requirement, we encourage unit tests for any code changes where possible.
🎁 If feasible, please check if an end-to-end test can be added. See README for more details.
🔎 If there's any flakiness in the CI tests, don't necessarily ignore it. It's better to address the issue before merging, or provide a valid reason to bypass it if fixing isn't possible (e.g., token rate limitations).

chmouel · 2024-09-11T06:58:08Z

/retest

codecov · 2024-09-11T07:03:27Z

Codecov Report

Attention: Patch coverage is 84.17722% with 25 lines in your changes missing coverage. Please review.

Project coverage is 65.13%. Comparing base (6f16e11) to head (9ba9da2).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/resolve/remote.go	78.07%	16 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1762      +/-   ##
==========================================
+ Coverage   65.08%   65.13%   +0.04%     
==========================================
  Files         174      174              
  Lines       13167    13185      +18     
==========================================
+ Hits         8570     8588      +18     
+ Misses       4029     4028       -1     
- Partials      568      569       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

savitaashture · 2024-09-13T16:11:09Z

@piyush-garg Thank you for the PR

Can you add e2e to cover the scenario which is described in this https://issues.redhat.com/browse/SRVKP-4018 and https://issues.redhat.com/browse/SRVKP-3517 which gives confidence that PR works in those cases
Will this PR covers this issue https://issues.redhat.com/browse/SRVKP-3973 ?

savitaashture · 2024-09-14T13:56:24Z

pkg/resolve/remote.go

+			if pipelinerun.GetObjectMeta().GetAnnotations() == nil {
+				continue
+			}


We have this check below

if len(pipelinerun.GetObjectMeta().GetAnnotations()) == 0 { continue }

can we avoid this nil check ??

first i am checking if it is nil or not, then if it is not nil then checking if it has any key value pairs.

these checks i have kept, same like before, it were there in old resolver too

pkg/resolve/remote.go

savitaashture · 2024-09-14T14:08:18Z

pkg/resolve/remote.go

@@ -32,74 +31,161 @@ func alreadySeen[T NamedItem](items []T, item T) bool {
 //
 // The precedence logic for Pipeline is first from PipelineRun annotations and
 // then from Tekton directory.
-func getRemotes(ctx context.Context, rt *matcher.RemoteTasks, types TektonTypes) (TektonTypes, error) {
-	remoteType := &TektonTypes{}
+func resolveRemoteResources(ctx context.Context, rt *matcher.RemoteTasks, types TektonTypes, ropt *Opts) ([]*tektonv1.PipelineRun, error) {


This function is too big can we split into smaller function specific to task and pipeline

piyush-garg · 2024-09-16T05:16:57Z

/test

savitaashture · 2024-09-16T09:03:31Z

@piyush-garg
error message

[notice] A new release of pip is available: 23.2.1 -> 24.2
[notice] To update, run: pip install --upgrade pip
3: B6 Body message is missing

chmouel · 2024-09-16T09:33:56Z

/retest

piyush-garg · 2024-09-17T04:01:10Z

@piyush-garg Thank you for the PR

Can you add e2e to cover the scenario which is described in this https://issues.redhat.com/browse/SRVKP-4018 and https://issues.redhat.com/browse/SRVKP-3517 which gives confidence that PR works in those cases

Will this PR covers this issue https://issues.redhat.com/browse/SRVKP-3973 ?

Added e2e tests for all three

This will redesign resolver to work with multiple edge case scenarios. Previously pac resolver was filtering pipelinerun based on annotations but it was resolving all pipelines in .tekton dir leading to resolving unnecessary pipelines and other issue was it was storing task based on task name, instead of annotation name and version, so different version of task were not used across pipelineruns in .tekton dir Now with new design we are first filtering pipelinerun based on annotations, and then processing all pipelineruns one by one and only resolving pipeline related to these pipelineruns. Also we are now maintaining map of tasks with name and version at event level to not re fetch the task and now inline spec replacement in pipelinerun is done one by one so respective task as mentioned in annotation with name and version is used Also before filtering the pipelineruns, we should make sure that no two pipelineruns exists with same name in .tekton dir Added tests for the three scenarios

piyush-garg · 2024-09-17T06:29:55Z

/test

chmouel · 2024-09-16T11:57:08Z

pkg/resolve/remote.go

+				var task *tektonv1.Task
+				// if task is already fetched in the event, then just copy the task
+				if alreadyFetchedResource(fetchedResourcesForEvent.Tasks, remoteTask) {
+					rt.Logger.Debugf("skipping already fetched task %s in annotations on pipelinerun %s", remoteTask, pipelinerun.GetName())


Ok if i understand properly how the code flow goes, it's the first fetched task that wins? I think we want to document this in the docs as if there is two tasks with the same name the first grabbed from the .tektondir or the first one listed from the annotation (top to botoom i think?) will win, is that correct?

so i am trying to first get the task from if anything mentioned in annotations(first pipelinerun, then pipeline), if not then i try to see if there is task available with that name in .tekton dir

I am following this https://pipelinesascode.com/docs/guide/resolver/#tasks-or-pipelines-precedence

chmouel · 2024-09-19T11:52:20Z

LGTM

tested and reviewed this and this looks good, thanks @piyush-garg

piyush-garg force-pushed the refac_Resolver branch from 2ec4209 to ea9e50a Compare September 10, 2024 23:40

savitaashture reviewed Sep 14, 2024

View reviewed changes

piyush-garg mentioned this pull request Sep 16, 2024

Refactor Resolver #1757

Closed

7 tasks

piyush-garg force-pushed the refac_Resolver branch from 8eea80a to 10e1e9a Compare September 16, 2024 02:03

piyush-garg force-pushed the refac_Resolver branch from 1935bbb to f6089f8 Compare September 17, 2024 03:59

piyush-garg changed the title ~~Refactor Resolver~~ Redesign PAC Resolver Sep 17, 2024

piyush-garg force-pushed the refac_Resolver branch from f6089f8 to 9ba9da2 Compare September 17, 2024 04:04

chmouel reviewed Sep 17, 2024

View reviewed changes

chmouel merged commit d87fe97 into openshift-pipelines:main Sep 19, 2024
4 checks passed

github-actions bot mentioned this pull request Sep 20, 2024

[openshift-pipelines/pipelines-as-code] v0.28.0 vdemeester/chick-group#89

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign PAC Resolver #1762

Redesign PAC Resolver #1762

piyush-garg commented Sep 10, 2024 •

edited

Loading

chmouel commented Sep 11, 2024

codecov bot commented Sep 11, 2024 •

edited

Loading

savitaashture commented Sep 13, 2024

savitaashture Sep 14, 2024

piyush-garg Sep 16, 2024

piyush-garg Sep 16, 2024

savitaashture Sep 14, 2024

piyush-garg commented Sep 16, 2024

savitaashture commented Sep 16, 2024

chmouel commented Sep 16, 2024

piyush-garg commented Sep 17, 2024

piyush-garg commented Sep 17, 2024

chmouel Sep 16, 2024

piyush-garg Sep 18, 2024

piyush-garg Sep 18, 2024

chmouel commented Sep 19, 2024

Redesign PAC Resolver #1762

Redesign PAC Resolver #1762

Conversation

piyush-garg commented Sep 10, 2024 • edited Loading

Submitter Checklist

chmouel commented Sep 11, 2024

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

savitaashture commented Sep 13, 2024

savitaashture Sep 14, 2024

Choose a reason for hiding this comment

piyush-garg Sep 16, 2024

Choose a reason for hiding this comment

piyush-garg Sep 16, 2024

Choose a reason for hiding this comment

savitaashture Sep 14, 2024

Choose a reason for hiding this comment

piyush-garg commented Sep 16, 2024

savitaashture commented Sep 16, 2024

chmouel commented Sep 16, 2024

piyush-garg commented Sep 17, 2024

piyush-garg commented Sep 17, 2024

chmouel Sep 16, 2024

Choose a reason for hiding this comment

piyush-garg Sep 18, 2024

Choose a reason for hiding this comment

piyush-garg Sep 18, 2024

Choose a reason for hiding this comment

chmouel commented Sep 19, 2024

piyush-garg commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading