Skip to content

test(e2e): allow external PRs via ok-to-test label#2611

Open
zakisk wants to merge 1 commit intotektoncd:mainfrom
zakisk:add-check-for-external-users
Open

test(e2e): allow external PRs via ok-to-test label#2611
zakisk wants to merge 1 commit intotektoncd:mainfrom
zakisk:add-check-for-external-users

Conversation

@zakisk
Copy link
Copy Markdown
Member

@zakisk zakisk commented Mar 26, 2026

Add ok-to-test label gating as a fallback for external contributors who are not org members or repo collaborators. Maintainers can approve external PRs by adding the ok-to-test label after code review.

The label is automatically removed on synchronize, opened, and reopened events to prevent running CI on new untrusted code without re-approval.

Assisted-by: Claude Opus 4.6 (via Claude Code)

📝 Description of the Change

🔗 Linked GitHub Issue

Fixes #

🧪 Testing Strategy

  • Unit tests
  • Integration tests
  • End-to-end tests
  • Manual testing
  • Not Applicable

🤖 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.

  • I have not used any AI assistance for this PR.
  • I have used AI assistance for this PR.

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-by trailer to your commit message.
For example:

Co-authored-by: Claude noreply@anthropic.com

✅ Submitter Checklist

  • 📝 My commit messages are clear, informative, and follow the project's How to write a git commit message guide. The Gitlint linter ensures in CI it's properly validated
  • ✨ I have ensured my commit message prefix (e.g., fix:, feat:) matches the "Type of Change" I selected above.
  • ♽ I have run make test and make lint locally to check for and fix any
    issues. For an efficient workflow, I have considered installing
    pre-commit and running pre-commit install to
    automate these checks.
  • 📖 I have added or updated documentation for any user-facing changes.
  • 🧪 I have added sufficient unit tests for my code changes.
  • 🎁 I have added end-to-end tests where feasible. See README for more details.
  • 🔎 I have addressed any CI test flakiness or provided a clear reason to bypass it.
  • If adding a provider feature, I have filled in the following and updated the provider documentation:
    • GitHub App
    • GitHub Webhook
    • Gitea/Forgejo
    • GitLab
    • Bitbucket Cloud
    • Bitbucket Data Center

@gemini-code-assist
Copy link
Copy Markdown

Note

Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported.

@zakisk zakisk force-pushed the add-check-for-external-users branch from c119c8d to d0f87eb Compare March 26, 2026 12:32
@aThorp96
Copy link
Copy Markdown
Member

Do I understand right that this is to bring the ok-to-test label behavior closer in line with how PaC handles the /ok-to-test comment?

What restrictions do we have in place about labeling? I see I can't add labels on this PR so I assume there are some restrictions

@chmouel
Copy link
Copy Markdown
Member

chmouel commented Mar 26, 2026

it's a good idea but remember-ok-to-test is something we want to avoid which what this label is doing, if we want this we need to respect the remember-ok-to-test so every PR syncronization it will be automatically removed

@chmouel
Copy link
Copy Markdown
Member

chmouel commented Mar 26, 2026

Arf I was induce in error by Andrew email, (and looking at this one phone ) it's for our dogfooding, my comment is is still accurate tho, if we put the label it will stay like this and new iterations may introduce security issues

@zakisk
Copy link
Copy Markdown
Member Author

zakisk commented Mar 26, 2026

But it deletes label so how will it stay?

@aThorp96
Copy link
Copy Markdown
Member

@chmouel I believe this has the opposite effect; if a non-contributor updates a pull request which has the ok-to-test label this change will clear the label

@chmouel
Copy link
Copy Markdown
Member

chmouel commented Mar 26, 2026

ah yeah i see it properly now! okay yeah so it answer on labeled and remove it straight away... this sounds good to me, yeah labels are only for repo owner i think...

i know its a bit much but with claude and llm it should be easy can we take off this giant javascript thing and convert it in python in hack/ instead, this would make me more confident for reviews, check, linting etc...

Add ok-to-test label gating as a fallback for external contributors
who are not org members or repo collaborators. Maintainers can
approve external PRs by adding the ok-to-test label after code
review.

The label is automatically removed on synchronize, opened, and
reopened events to prevent running CI on new untrusted code without
re-approval.

Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
Assisted-by: Claude Opus 4.6 (via Claude Code)
@zakisk zakisk force-pushed the add-check-for-external-users branch from 62dab7d to b4c4dea Compare March 27, 2026 11:03
@zakisk
Copy link
Copy Markdown
Member Author

zakisk commented Mar 27, 2026

ah yeah i see it properly now! okay yeah so it answer on labeled and remove it straight away... this sounds good to me, yeah labels are only for repo owner i think...

i know its a bit much but with claude and llm it should be easy can we take off this giant javascript thing and convert it in python in hack/ instead, this would make me more confident for reviews, check, linting etc...

I extracted the script in .github/scripts and calling the exported func in e2e.yaml. if we do this in python script then it would lots of changes like passing the whole context and payload, creating github client etc this looks more convenient to me.

@chmouel
Copy link
Copy Markdown
Member

chmouel commented Mar 27, 2026

okay i ddin't know about .github/scripts,

one last thing were you able to properly test?

@zakisk
Copy link
Copy Markdown
Member Author

zakisk commented Mar 27, 2026

okay i ddin't know about .github/scripts,

one last thing were you able to properly test?

will do testing

@chmouel
Copy link
Copy Markdown
Member

chmouel commented Mar 27, 2026

Yeah, I'd like to make sure the label is removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants