fix: skip prun when target-ns repo cannot be found#2483
fix: skip prun when target-ns repo cannot be found#2483chmouel merged 1 commit intotektoncd:mainfrom
Conversation
When a PipelineRun carries a target-namespace annotation pointing to a namespace where no matching Repository object exists, resolveTargetNamespaceRepo was silently falling back to the originating repo. This caused foreign PipelineRuns (e.g. a /test no-match) to execute in the wrong namespace and pollute the Repository status, leading to flaky E2E assertions. This commits further fixes 606c830 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Summary of ChangesHello @chmouel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where PipelineRuns with a target-namespace annotation would incorrectly fall back to the original repository's namespace when the target repository could not be found. The core change modifies resolveTargetNamespaceRepo to return nil in such cases, allowing for more explicit handling. Consequently, pipeline execution is now correctly skipped, preventing runs in the wrong context. For cancellation operations, the logic now logs a warning and intentionally falls back to the original repository. The changes are logical, well-tested with new unit tests for both scenarios, and effectively resolve the described problem. The code is clean and I have no further recommendations.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where PipelineRuns with target-namespace annotations pointing to non-existent Repository objects would incorrectly fall back to executing in the originating namespace, causing namespace pollution and flaky E2E tests.
Changes:
- Modified
resolveTargetNamespaceRepoto returnnilinstead of falling back when target namespace repository is not found - Added logic to skip PipelineRun execution when target namespace repository is missing (for explicit /test commands)
- Added fallback behavior with warning for cancel operations when target namespace repository is missing
- Added comprehensive test coverage for both scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/pipelineascode/match.go | Modified resolveTargetNamespaceRepo to return nil on missing target repo; added check to skip PipelineRun when target repo not found |
| pkg/pipelineascode/cancel_pipelineruns.go | Added fallback logic to use original repo when target namespace repo is missing during cancel operations |
| pkg/pipelineascode/match_test.go | Added test case for skipping PipelineRun when target namespace repo not found |
| pkg/pipelineascode/cancel_pipelineruns_test.go | Added test to verify fallback behavior during cancel operations when target namespace repo is missing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 Description of the Change
This fix addresses a critical issue in PipelineRun handling when target namespace repositories are not found. The problem was when a PipelineRun carries a
target-namespaceannotation pointing to a namespace where no matching Repository object exists, the code was silently falling back to the originating repository. This caused:/test no-match) to execute in the wrong namespaceWhy the original fix in 606c830 needed this follow-up
The
resolveTargetNamespaceRepofunction introduced in 606c830 was written with a defensive fallback philosophy: if anything goes wrong (lookup error, repo not found), return thefallbackReposo the system keeps working. The assumption was that it's better to run in the wrong namespace than to fail entirely.However, this removed the caller's ability to distinguish between "resolved successfully" and "failed, gave you the fallback", and the silent fallback did the wrong thing depending on which code path was calling:
/testpath (getPipelineRunsFromRepo): If a PipelineRun template saystarget-namespace: "production"but no Repository CR exists in that namespace, the old code would silently run the PipelineRun using the source repo — executing it in the wrong namespace with no indication. The user thinks it's targetingproduction, but it's not. This is a silent misconfiguration that should be surfaced, not hidden./cancelpath (resolveRepoForTargetCancelPipelineRun): Here, falling back is actually fine — you still want to cancel the running PipelineRun even if the target-namespace repo is gone. But the old code had no logging to indicate the fallback happened.This fix changes
resolveTargetNamespaceRepoto returnnilon failure, pushing the decision back to each caller:/testnow skips the PipelineRun and emits an event, while/cancelstill falls back but logs a warning.👨🏻 Linked Jira
🔗 Linked GitHub Issue
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
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: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix any issues. For an efficient workflow, I have considered installing pre-commit and runningpre-commit installto automate these checks.