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

Separate Bot User GitHub Tokens from regro GitHub Tokens #3326

Closed
wants to merge 3 commits into from

Conversation

ytausch
Copy link
Contributor

@ytausch ytausch commented Dec 6, 2024

Description:

Our proposed integration tests for #261 need to interact with the GitHub API with multiple different identities in a staging environment. Since nowadays, fine-grained personal access tokens are recommended by GitHub whenever possible, they clearly separate access to different GitHub users/organizations.

This means the bot needs to support using different access tokens depending on which GitHub user/org it is interacting with. This PR contributes the necessary changes.

Essentially, BOT_TOKEN can now be filled with a fine-grained access token for regro-cf-autotick-bot and REGRO_TOKEN with a fine-grained token for regro.

Note that these changes do not prevent from continuing to run the bot with a classic access token in production.

I carefully reviewed the source code to verify that the correct tokens are available and used everywhere. It would be helpful if you, @beckermr, could double-check.

I leave it up to you if you also want to update the production tokens to fine-grained tokens together with these changes. It involves more risk of breaking the bot but would ensure that future source code changes correctly differentiate between the two tokens. We can also wait for the integration tests to have more assurance here.

@pavelzw

Checklist:

  • Pydantic model updated or no update needed

Cross-refs, links to issues, etc:

@ytausch ytausch requested a review from beckermr December 6, 2024 18:59
Comment on lines -48 to -49
env:
BOT_TOKEN: ${{ secrets.AUTOTICK_BOT_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed BOT_TOKEN for install_bot_code.sh entirely everywhere because I could not find where it uses the token.

@@ -81,7 +80,7 @@ jobs:
export RUN_URL="https://github.com/regro/cf-scripts/actions/runs/${RUN_ID}"
conda-forge-tick deploy-to-github
env:
BOT_TOKEN: ${{ secrets.AUTOTICK_BOT_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explanation: This only pushes to cf-graph so the permissions for regro are sufficient

@ytausch ytausch force-pushed the separate-bot-tokens branch from 2867340 to d9f91ee Compare December 6, 2024 19:03
@ytausch ytausch marked this pull request as ready for review December 6, 2024 19:03
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 38.77551% with 30 lines in your changes missing coverage. Please review.

Project coverage is 76.40%. Comparing base (4ba3b0b) to head (2ca2b70).
Report is 39 commits behind head on main.

Files with missing lines Patch % Lines
conda_forge_tick/git_utils.py 27.27% 16 Missing ⚠️
tests/conftest.py 0.00% 7 Missing ⚠️
conda_forge_tick/events/push_events.py 0.00% 2 Missing ⚠️
conda_forge_tick/lazy_json_backends.py 0.00% 2 Missing ⚠️
tests/test_lazy_json_backends.py 33.33% 2 Missing ⚠️
conda_forge_tick/all_feedstocks.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3326      +/-   ##
==========================================
- Coverage   76.42%   76.40%   -0.03%     
==========================================
  Files         130      130              
  Lines       14234    14255      +21     
==========================================
+ Hits        10879    10891      +12     
- Misses       3355     3364       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.github/workflows/bot-feedstocks.yml Outdated Show resolved Hide resolved
.github/workflows/bot-events.yml Show resolved Hide resolved
Copy link
Contributor

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I don't follow the need for this change. I thought the plan was to put testing feedstocks in the regro org directly and let the bot fork them, in which case we can use one token and things are a lot simpler.

@ytausch
Copy link
Contributor Author

ytausch commented Dec 6, 2024

I don't follow the need for this change. I thought the plan was to put testing feedstocks in the regro org directly and let the bot fork them, in which case we can use one token and things are a lot simpler.

I know we talked about this already so sorry if I confused you.

The test environment needs three things:

  1. A copy of cf-graph-countyfair which is not the real one.
  2. A bot user account which is not the real bot user account.
  3. A GitHub org containing the mock feedstocks.

It is possible to put the mock cf-graph (1) in regro if the permissions are restricted correctly.

Mock feedstocks or forks thereof (2/3) should not be put in regro or any other existing org: The test setup needs permission to create and delete arbitrary repositories for the owners of the feedstocks and forks. The test setup should not be able to delete production resources such as cf-graph.

Even if the mock graph is put into regro, 2 and 3 require that there is a separation between actions on the bot user account and regro.

@beckermr
Copy link
Contributor

beckermr commented Dec 6, 2024

It seems a lot easier to start with something simpler. Making and then deleting repos for running the tests is slow and error prone. We can post some notes on how to set things up and do most of it by hand.

@ytausch
Copy link
Contributor Author

ytausch commented Dec 6, 2024

It seems a lot easier to start with something simpler. Making and then deleting repos for running the tests is slow and error prone. We can post some notes on how to set things up and do most of it by hand.

Automatic test environment creation is already implemented. And it's surely not a bad thing that testing is as much automated as possible. Did not see that anything is slow or raises errors. So I'd appreciate if this PR moves forward.

@ytausch ytausch requested a review from beckermr December 8, 2024 21:23
@ytausch ytausch marked this pull request as draft December 9, 2024 12:43
@ytausch
Copy link
Contributor Author

ytausch commented Dec 9, 2024

Converting to draft because a GitHub app could solve this issue in another way.

@ytausch
Copy link
Contributor Author

ytausch commented Dec 9, 2024

We decided to use a classic PAT for now, although GitHub discourages their usage, making this PR redundant. There are reasons though why we want to migrate the bot to use GitHub app authentication eventually.
Let's discuss the exact distribution of repositories separately! Happy to set up a call or use the next bot sync for that. I will reach out to you as soon as we have something to show.

@ytausch ytausch closed this Dec 9, 2024
@beckermr
Copy link
Contributor

beckermr commented Dec 9, 2024

The bot cannot fully use a GitHub app due to GitHub apps not being able to fork feedstocks. We manage a mix of apps and machine users on conda-forge and it is a pain. The only reason we do it there is for the increased api request limits. If we do not need those limits here, then the complexity is not worth it.

@ytausch
Copy link
Contributor Author

ytausch commented Dec 9, 2024

I double-checked and found that GitHub apps indeed can't be used to fork repositories into a user account, but I could successfully fork into an organization if the app is installed in an organization. It requires the original repo to be public, but this should not be a concern for us. Nothing should speak against cf-regro-autotick-bot being an organization, right?

I agree that the increased rate limits are the main reason why we should consider using a GitHub app.

@beckermr
Copy link
Contributor

beckermr commented Dec 9, 2024

The bot is going to remain a machine user and not be changed to an org.

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.

2 participants