fix: ignore bot users for blocking pull request when non authorized #1982
Conversation
There was a problem hiding this comment.
PR Overview
This PR implements a workaround to ignore bot users (usernames ending in "[bot]") to prevent conflicts between PaC and prow during GitOps commands.
- Added a CheckBot function with a corresponding regex-based rule and unit tests in pkg/acl.
- Updated the docs to explain how bot users are handled.
- Modified the access check in Pipelines-as-Code to silently ignore bot users when determining permissions.
Reviewed Changes
| File | Description |
|---|---|
| pkg/acl/robots.go | Introduced a CheckBot function with regex-based bot detection. |
| pkg/acl/robots_test.go | Added unit tests for various sender names to validate bot detection. |
| docs/content/docs/guide/running.md | Updated the documentation to describe bot exemptions in PipelineRun. |
| pkg/pipelineascode/match.go | Added a conditional check to bypass status creation for bot users. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
|
/label bug |
|
✅ Added labels: bug. |
|
used the boussole to generate and then grabe the payload and make sure the type is Bot for a bot |
chmouel
force-pushed
the
SRVKP-7242-prevent-bogus-konflux-checks-blocking-merges
branch
from
7c8a7cf to
c9a5bbd
Compare
There was a problem hiding this comment.
PR Overview
This PR introduces logic to ignore status updates for GitHub bot users, updates documentation to explain the behavior, and expands test coverage for bot scenarios. Key changes include updates in status creation logic to bypass bot requests, adjustments to event payload parsing to capture the user type, and modifications to tests to verify the new behavior.
Reviewed Changes
| File | Description |
|---|---|
| docs/content/docs/guide/running.md | Updated documentation to describe bot exemption behavior |
| pkg/provider/github/status.go | Added condition to skip setting status for bot users |
| pkg/provider/github/status_test.go | Extended tests to include bot user scenarios |
| pkg/provider/github/github.go | Added a new field for user type |
| pkg/pipelineascode/match.go | Marked statuses with AccessDenied for unauthorized users |
| pkg/provider/interface.go | Added AccessDenied field to StatusOpts |
| pkg/provider/github/parse_payload.go | Updated payload parsing to capture sender user type |
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
pkg/provider/github/status.go:350
- [nitpick] Consider improving the error message for clarity, e.g. 'cannot set status on GitHub: no token or URL provided'.
return fmt.Errorf("cannot set status on github no token or url set")
c9a5bbd to
056ce7e
Compare
chmouel
changed the title
|
Is it possible to spoof the |
|
@arewm that is not possible, if malicious user was able to spoof the github payload then it would be a severe security issue. We enforce using webhook payload secret for that reason... |
chmouel
force-pushed
the
SRVKP-7242-prevent-bogus-konflux-checks-blocking-merges
branch
from
056ce7e to
3a758ea
Compare
|
/test |
still failing 🤔 |
|
/retest |
|
It fails on the first time as always.... |
|
@chmouel can you please see my comments, otherwise nothing is left that stopping merge. |
- Added logic to skip setting status for bot users in `CreateStatus` method. - Updated `processEvent` to capture user type from GitHub events. - Modified tests to include scenarios for bot users. - Corrected minor formatting issues in documentation. More details here: https://issues.redhat.com/browse/SRVKP-7242 Signed-off-by: Chmouel Boudjnah <[email protected]>
chmouel
force-pushed
the
SRVKP-7242-prevent-bogus-konflux-checks-blocking-merges
branch
from
3a758ea to
953dc6b
Compare
|
lmk wdyt @zakisk |
|
/lgtm |
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congrats @chmouel your PR Has been approved 🎉
✅ Pull Request Approved
Approval Status:
- Required Approvals: 1
- Current Approvals: 1
👥 Reviewers Who Approved:
| Reviewer | Permission Level | Approval Status |
|---|---|---|
| @zakisk | admin |
✅ |
📝 Next Steps
- Ensure all required checks pass
- Comply with branch protection rules
- Request a maintainer to merge using the
/mergecommand (or merge it
directly if you have repository permission).
Automated by the PAC Boussole 🧭
|
/merge |
pipelines-as-code
bot
merged commit ccea763
into
openshift-pipelines:main
✅ PR Successfully Merged
Approvals Summary:
Thank you @chmouel for your valuable contribution! 🎉 Automated by the PAC Boussole 🧭 |
chmouel
deleted the
SRVKP-7242-prevent-bogus-konflux-checks-blocking-merges
branch
CreateStatusmethod.processEventto capture user type from GitHub events.More details here: https://issues.redhat.com/browse/SRVKP-7242
Changes
Submitter Checklist
📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).
♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.
✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).
📖 Document any user-facing features or changes in behavior.
🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.
🎁 If feasible, add an end-to-end test. See README for details.
🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).
(update the documentation accordingly)