Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-38315: [Dev][CI] autotune needs additional permissions to push to PR branches #38523

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thisisnic
Copy link
Member

@thisisnic thisisnic commented Oct 31, 2023

Rationale for this change

autotune is giving 403 errors as there aren't enough permissions on the default GITHUB_TOKEN

What changes are included in this PR?

Add required permissions

Are these changes tested?

I've pushed some dodgy formatting to this branch, so am intend to test it here

Are there any user-facing changes?

Nope

@thisisnic
Copy link
Member Author

@github-actions autotune

@github-actions
Copy link

⚠️ GitHub issue #38315 has been automatically assigned in GitHub to PR creator.

@thisisnic
Copy link
Member Author

thisisnic commented Oct 31, 2023

Looks like this fix didn't work. It looks like the token still didn't have the correct permissions though.

@assignUser - you grok this stuff better than me; would I expect autotune to be using the version in this PR, or the version in main? If it's the former, we need to do something with the pr-push step, but if the latter then this might be sufficient (and I'll just get rid of my arbitrary formatting change).

@kou
Copy link
Member

kou commented Oct 31, 2023

We can't test the comment_bot.yml workflow with PR.
Because the workflow always uses the configuration in the default branch.

Could you test this on your fork? (You can test this by pushing a change to the default branch.)

@thisisnic
Copy link
Member Author

Thanks for the suggestion @kou! Now got this working on my fork.

@assignUser
Copy link
Member

The rebase job will also need the new permission to work again :) I would prefer it if there was a more restrictive permission we could use for this but it looks like this is the only one that will work...

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Could someone (except thisisnic) try this on https://github.com/thisisnic/arrow just in case? (It may work only by the repository owner.)

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Nov 2, 2023
@assignUser
Copy link
Member

Good instinct @kou https://github.com/thisisnic/arrow/actions/runs/6741053064/job/18325048732#step:13:15
Maybe it's another permission after all? If so I was unable to figure it out using the gh docs...

Also thinking about this in detail, I think we should either remove the rebase part of the job (or gate it to be only usable by the pr creator and maybe committers. As it is now anyone could do a drive by comment a have the action force push to a branch. The danger is not huge but it feels a bit invasive... 🤷

@assignUser
Copy link
Member

But I did find out that there is now a way to rebase the fork branch via gui which is imo a good replacement for the rebase protion of this job: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/keeping-your-pull-request-in-sync-with-the-base-branch

@jonkeane
Copy link
Member

jonkeane commented Nov 3, 2023

@assignUser Would you mind making a separate issue for the rebase stuff so we don't mix the two up here.

As for the write permissions — it's possible that github has now generally restricted that? Reading the docs

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception to this behavior is where an admin user has selected the Send write tokens to workflows from pull requests option in the GitHub Actions settings. For more information, see "Managing GitHub Actions settings for a repository."

Which seems to suggest that we cannot add the write permission unless it's already set org-wide. Or possibly there is a setting we can change in the .asf.yml that would allow this?

@assignUser
Copy link
Member

@jonkeane well it is closely related, if we don't manage to get the permission working (which is the case based on you findings) there will also not be a way to do the rebase and we can remove both steps in this PR.

Good catch with the docs, this is def. something that worked previously but it seems that they removed this possibility with the rework of default permissions this year. The setting mentioned above is something we/INFRA will never enable for security reasons as it would remove the protections that the pull_request trigger gives us and allow for all kinds of bad things to happen^^ IIUC this is meant for use within orgs for private repos where you want to use private forks instead of branches.

So the conclusion seems to be that this is not possible anymore in an automated way? Individuals with write access to the base repo(apache/arrow) can still manually push changes to the fork branch if the checkmark 'Maintainers are allowed to edit this pull request.' is set but the generated GITHUB_TOKEN's can't? + changes via web gui. :/

@thisisnic
Copy link
Member Author

If this is a no-go, should I just remove both of these in this PR? They were nice to have, but as a maintainer reviewing PRs I'm happy enough to just pull locally and style or rebase something myself, or tell the contributor to run styler on their PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge Awaiting merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dev][CI] autotune failing to push to PR branches?
4 participants