feat(matcher): skip ACL when .tekton dir absent#2699
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the repository matching and ACL check logic in matchRepoPR by decomposing it into setupRepo, fetchTektonTemplates, and checkUserAccess. This change ensures that ACL checks and status updates are skipped if no .tekton templates are found. Feedback was provided regarding error handling: specifically, ensuring the Repository object is returned upon failure in matchRepoPR to assist in error reporting, and avoiding the suppression of errors in fetchTektonTemplates to prevent masking infrastructure or API issues.
| p.debugf("matchRepoPR: starting repo setup for url=%s", p.event.URL) | ||
| repo, err := p.setupRepo(ctx) | ||
| if err != nil { | ||
| return nil, nil, err |
There was a problem hiding this comment.
When setupRepo returns an error, it may still have successfully matched a Repository object. Returning nil for the repository here prevents the caller from using that repository information for error reporting or status updates. It is better to return the matched repo along with the error.
| return nil, nil, err | |
| return nil, repo, err |
| msg = fmt.Sprintf("cannot locate templates in %s/ directory for this repository in %s", tektonDir, p.event.HeadBranch) | ||
| } | ||
| p.eventEmitter.EmitMessage(nil, logLevel, reason, msg) | ||
| return "", nil |
There was a problem hiding this comment.
Returning "", nil when err != nil (checked at line 205) swallows potential infrastructure or API errors (e.g., rate limits, network failures). This makes it difficult for users to understand why their pipelines are not triggering. The function should distinguish between a "directory not found" condition and other errors, returning the latter to the caller.
| return "", nil | |
| return "", err |
There was a problem hiding this comment.
This is pre-existing behavior, the original code returned nil, nil here (previously on the combined err != nil || rawTemplates == "" path). The error is not silently discarded; it's emitted at ErrorLevel using eventEmitter.EmitMessage with reason RepositoryInvalidPipelineRunTemplate, so it is visible on the cluster.
Propagating the error here would cause matchRepoPR call to post a faiulure status on the commit - which is aa bheaviour change. @zakisk could you provide some context on this behaviour?
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2699 +/- ##
==========================================
+ Coverage 59.00% 59.05% +0.04%
==========================================
Files 208 208
Lines 20436 20477 +41
==========================================
+ Hits 12059 12092 +33
- Misses 7603 7607 +4
- Partials 774 778 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Reorder matchRepoPR to fetch .tekton templates before ACL checks. When the .tekton directory is missing, the flow returns early without triggering permission checks or posting unwarranted status updates to the Git provider. Split verifyRepoAndUser into setupRepo and checkUserAccess, and extract fetchTektonTemplates from getPipelineRunsFromRepo. The existing verifyRepoAndUser is kept as a thin wrapper for flows that need setup + ACL without .tekton checks. Signed-off-by: Akshay Pant <akpant@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
theakshaypant
force-pushed
the
feat/skip-neutral-status-on-absent-tekton-dir
branch
from
7ee69df to
b02e485
Compare
|
changes looks fine but I have concern about API calling, let's assume a scenario where a repo has 100 pipeline runs and an unauthorized user sends PR, PaC will iterate through all the pipeline run and then will check the permission. it won't make issue when there is no pipeline run, but a repo with 100 pipeline run or other tekton resources (taskruns, stepactions) there will api calls equals to resources which exhaust the rate limit of token and after all that it will just put a message that use is not authorized to run CI 🤷🏻 |
|
there no issue of having neutral check on |
That is a fair concern, let me rethink and update here.
Agreed, have retained this original behaviour from 846416e |
3 participants
📝 Description of the Change
Reorder matchRepoPR to fetch .tekton templates before ACL checks. When the .tekton directory is missing, the flow returns early without triggering permission checks or posting unwarranted status updates to the Git provider.
Split verifyRepoAndUser into setupRepo and checkUserAccess, and extract fetchTektonTemplates from getPipelineRunsFromRepo. The existing verifyRepoAndUser is kept as a thin wrapper for flows that need setup + ACL without .tekton checks.
🔗 Linked GitHub Issue
Fixes #2692
🧪 Testing Strategy
No PaC check run on external contributor when
.tektondir is absentNeutral check still shows up if an admin comments
/ok-to-testwith.tektondir absentController Logs
{"level":"debug","ts":"2026-04-27T06:13:43.427Z","logger":"pipelinesascode","caller":"pipelineascode/logging.go:9","msg":"fetchTektonTemplates: fetched templates length=0","commit":"cb3975c-dirty","provider":"github","event-id":"3cb67620-4200-11f1-9135-e12bf898c03c","event-sha":"a85ee1193878a13898d1a24c64b1af91dda86ee1","event-type":"pull_request","source-repo-url":"https://ghe.pipelinesascode.com/akpant-test/ghe-app-test","target-branch":"main","source-branch":"nonadmin-patch-1","namespace":"default"} {"level":"info","ts":"2026-04-27T06:13:43.427Z","logger":"pipelinesascode","caller":"events/emit.go:51","msg":"cannot locate templates in .tekton/ directory for this repository in nonadmin-patch-1","commit":"cb3975c-dirty","provider":"github","event-id":"3cb67620-4200-11f1-9135-e12bf898c03c","event-sha":"a85ee1193878a13898d1a24c64b1af91dda86ee1","event-type":"pull_request","source-repo-url":"https://ghe.pipelinesascode.com/akpant-test/ghe-app-test","target-branch":"main","source-branch":"nonadmin-patch-1","namespace":"default"} {"level":"debug","ts":"2026-04-27T06:13:43.427Z","logger":"pipelinesascode","caller":"pipelineascode/logging.go:9","msg":"match results: matched=0 repo=default/akshay-pac-test-repo","commit":"cb3975c-dirty","provider":"github","event-id":"3cb67620-4200-11f1-9135-e12bf898c03c","event-sha":"a85ee1193878a13898d1a24c64b1af91dda86ee1","event-type":"pull_request","source-repo-url":"https://ghe.pipelinesascode.com/akpant-test/ghe-app-test","target-branch":"main","source-branch":"nonadmin-patch-1","namespace":"default"} {"level":"debug","ts":"2026-04-27T06:13:43.427Z","logger":"pipelinesascode","caller":"pipelineascode/logging.go:9","msg":"no pipelineruns matched; returning without starting any runs","commit":"cb3975c-dirty","provider":"github","event-id":"3cb67620-4200-11f1-9135-e12bf898c03c","event-sha":"a85ee1193878a13898d1a24c64b1af91dda86ee1","event-type":"pull_request","source-repo-url":"https://ghe.pipelinesascode.com/akpant-test/ghe-app-test","target-branch":"main","source-branch":"nonadmin-patch-1","namespace":"default"}🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.