-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: Ability to configure repo Collaborators (teams + users) #232
feat: Ability to configure repo Collaborators (teams + users) #232
Conversation
* chore: Testing pygithub methods * fix: Use pygithub methods * chore: Linting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good and verified to be working
@andrewthetechie - after you approve this one, I will do the variables one, and then the environment one - because the environment one is dependent on variables and secrets |
|
@andrewthetechie - can you start reviewing these? I have them all queued up and ready to go, including removing REST API calls that were in the code where pygithub has a method to use. |
@actuarysailor this isn't passing CI https://github.com/andrewthetechie/gha-repo-manager/actions/runs/9313429181/job/25635812531?pr=232 and the test is running. Some change in your PR has changed/broken how diff detection is working. Here's a passing run on main https://github.com/andrewthetechie/gha-repo-manager/actions/runs/9275724358/job/25687709626 |
@andrewthetechie - Ok, I will take a look. I am suspecting it is because I was trying to allow it to run with certain YAML nodes are omitted which causes me problems on my end. |
@andrewthetechie - it's the same problem I've mentioned before. So while it now runs because it coalesces to github.token, github.token has limited OAUTH scopes compared to your PAT. And if I define the PAT in my repo, it does not have permission to see your repo differences. The only way to get the diffs check to pass would be to update your test workflow to be pull_request_target instead of pull_request. Either that, or we need to modify a bunch of files I have not yet modified to handle the various cases of OAUTH scopes on the tokens provided. |
@andrewthetechie - converting back to draft to add in some OAUTH conditions |
@andrewthetechie - everything is passing/working at least as well as it was before this PR. I think we still may have a token issue with the branch_protection test for diffs, but will need to see it run after you merge in your repo. This is the issue I keep bringing up in #51 that you said you are unable to recreate; if you look at the action test you will see the error I logged in #51 but I currently have it passing it when that error happens... I think it is because with your PAT, the error does not happen, but with mine it does... Also, I have noticed that some things work a bit differently with the new FINE GRAINED PAT TOkens... Your action may require updates when people use those types of tokens... |
@@ -309,7 +311,12 @@ def check_repo_branch_protections( | |||
diff_protections[config_bp.name] = ["Branch is not protected"] | |||
continue | |||
|
|||
this_protection = repo_bp.get_protection() | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewthetechie this is where #51 is occurring, and has been since I first opened it.
"enable_automated_security_fixes", | ||
"enable_vulnerability_alerts", | ||
]: | ||
if repo._requester.oauth_scopes is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewthetechie - This works with the old PAT types, like what you are using now, but the new PAT types always have oauth_scopes of NONE it seems... This is where if i use a new PAT type or the github.token diffs get falsely displayed because the oauth denies access to these settings and incorrectly flags them as different because the oauth denial returns NONE instead of an error.
actions_toolkit.set_output("diff", json_diff) | ||
|
||
if inputs["action"] == "check": | ||
if not check_result: | ||
actions_toolkit.info(inputs["repo_object"].full_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewthetechie - if you want to, you can remove these two lines I added... Was just trying to get details on what differences were being flagged so I knew which module to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, I tried using the new debug methodology GitHub has: https://docs.github.com/en/actions/monitoring-and-troubleshooting-workflows/enabling-debug-logging
But I was unable to see any debug messages... Maybe action_toolkit debug method not compatible with the new changes to GitHub debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewthetechie - please review and approve so I can get the other two PRs updated from your main and remove draft status on them.
@andrewthetechie not really, the issue is actually from repo permission differences due to token differences. |
@actuarysailor I merged this PR and it broke CI https://github.com/andrewthetechie/gha-repo-manager/actions/runs/9339177635 I'm going to revert the merge. Unfortunately, I don't think I can accept anymore PRs to this repo until CI can be trusted to work on PRs. I don't have time to chase down fixing stuff after merges to main. If you can figure out how to fix the CI to run on your PRs from forks, I'm happy to continue to look at them. Changing to pull_request_target does not seem to fix the issue - instead it breaks any runs not coming from a PR. Otherwise, I'd suggest just forking this action and maintaining your own copy. I don't need any of these features your adding for my own use, so I am not able to dedicate time to trying to write/maintain them. |
Ok, can I just update the test to do pull_request_target in a separate PR and then we can verify this test actually works in PRs? I can't debug how it acts in your repo without that. |
Should be an easy check, results from this PR test should match what you saw post merge before revert. Then you can have confidence once or fixed you get same results. |
Description
Updated the action to enable the configuration of:
Motivation and Context
I manage several hundred repositories at work, and we use templates to ensure that everyone has the correct CI/CD pipelines for various project types varying by platform, programming language, and teams responsible for them. While this action does not fully automate everything such that all repositories require no manual configurations, it does streamline the configuration process for new repositories, such that:
While new repositories still need their settings.yml file updated for any customizations specific to a new repository, this allows even non-admins to propose those updates via a pull request. Secret values may still need a manual configuration because it is not guaranteed that:
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projects