fix: target-namespace annotation for gitops /test, /cancel comments#2472
fix: target-namespace annotation for gitops /test, /cancel comments#2472chmouel merged 2 commits intotektoncd:mainfrom
Conversation
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 refines how explicit 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
|
🔍 PR Lint Feedback
|
There was a problem hiding this comment.
Code Review
This pull request correctly implements support for the target-namespace annotation for /test and /cancel commands, ensuring that explicit commands can target resources across namespaces. The changes are well-structured, with new functions to handle the repository resolution logic for both commands, and the addition of new tests provides good coverage for the new functionality. I have a few suggestions to improve code conciseness and reduce duplication, including addressing a redundant check as per repository guidelines, but overall the changes are solid.
There was a problem hiding this comment.
Pull request overview
This PR fixes target-namespace resolution for explicit /test and /cancel commands in Pipelines as Code. When a PipelineRun template specifies a target-namespace annotation, these commands now correctly use the annotated namespace instead of always falling back to the URL-matched repository namespace. This addresses cross-namespace scenarios where templates are deployed in different namespaces than the repository that triggered them.
Changes:
- Added
resolveTargetNamespaceRepofunction to resolve the target repository based on the target-namespace annotation for/testcommands - Added
resolveRepoForTargetCancelPipelineRunfunction to resolve the target repository for/cancelcommands by parsing templates - Added
pipelineRunIdentifierhelper function for consistent pipeline run identification in logging - Added comprehensive test coverage for both
/testand/cancelcommand scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/pipelineascode/match.go | Implements target-namespace resolution logic for explicit /test commands, adds helper functions for PR identification and namespace resolution |
| pkg/pipelineascode/match_test.go | Adds test coverage for /test command with and without target-namespace annotation |
| pkg/pipelineascode/cancel_pipelineruns.go | Implements target-namespace resolution for /cancel commands by fetching and parsing templates |
| pkg/pipelineascode/cancel_pipelineruns_test.go | Adds test coverage for /cancel command resolving target namespace from templates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e45a0c1 to
34f3a0e
Compare
c443c48 to
a6d0b01
Compare
|
/review |
|
/gemini review |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
There was a problem hiding this comment.
Code Review
This pull request enhances the handling of /test and /cancel commands by respecting the target-namespace annotation in PipelineRun templates. However, it introduces critical cross-namespace authorization bypass vulnerabilities, as the code fails to re-verify user permissions against the newly resolved Repository CR after resolving the target-namespace annotation, potentially allowing unauthorized operations. Furthermore, a critical security vulnerability exists in the GitHub App's comment handling, as it does not verify the author of comments it attempts to modify, potentially allowing unauthorized edits. The review also suggests improvements for code maintainability and consistency by extracting duplicated logic and harmonizing template resolution between /test and /cancel commands.
d91679b to
0f7b7c6
Compare
Fix target-namespace resolution for explicit `/test` and `/cancel` commands. When a PipelineRun template specifies a `target-namespace` annotation, these explicit commands now use that annotated namespace instead of falling back to the URL-matched repository. This resolves cross-namespace issues where explicit commands targeted templates in different namespaces than the URL-derived repository, while maintaining backward compatibility for templates without annotations. Assisted-by: Claude Sonnet 4.5 (via Claude Code) Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
The logic for updating and creating comments on GitHub pull requests was updated to better handle scenarios with duplicate comments. Previously, the system might create multiple identical comments or fail to correctly update an existing one. This change refactored the comment handling to first retrieve all comments matching a marker. If duplicates are found, it now prioritizes one comment to update and deletes the others. Additionally, a jittered retry mechanism was introduced when creating new comments to reduce the chance of race conditions where multiple processors might simultaneously create duplicate comments. The test suite was also updated to cover these deduplication scenarios. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
0f7b7c6 to
88e08bc
Compare
Fix target-namespace resolution for explicit
/testand/cancelcommands. When a PipelineRun template specifies atarget-namespaceannotation, these explicit commands now use that annotated namespace instead of falling back to the URL-matched repository.This resolves cross-namespace issues where explicit commands targeted templates in different namespaces than the URL-derived repository, while maintaining backward compatibility for templates without annotations.
📝 Description of the Change
👨🏻 Linked Jira
🔗 Linked GitHub Issue
Fixes #
🚀 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 anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.