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

feat: Ability to configure repo Collaborators (teams + users) #232

Merged

Conversation

actuarysailor
Copy link
Contributor

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:

  • Baseline permissions for contributors get automatically set
  • Baseline configurations for common settings get automatically set

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:

  • the worker running this action would have the appropriate values stored in an environment variable
  • hard-coding secret values into the settings.yml file directly defeats the purpose of having an encrypted repo secret

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects

@actuarysailor actuarysailor changed the title feat: Collaborators feat: Ability to configure repo Collaborators (teams + users) May 13, 2024
* chore: Testing pygithub methods

* fix: Use pygithub methods

* chore: Linting
Copy link
Contributor Author

@actuarysailor actuarysailor left a 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

@actuarysailor actuarysailor marked this pull request as ready for review May 31, 2024 04:58
@actuarysailor
Copy link
Contributor Author

@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

@actuarysailor
Copy link
Contributor Author

@actuarysailor
Copy link
Contributor Author

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

@andrewthetechie
Copy link
Owner

@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

@actuarysailor
Copy link
Contributor Author

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

@actuarysailor
Copy link
Contributor Author

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

@actuarysailor
Copy link
Contributor Author

@andrewthetechie - converting back to draft to add in some OAUTH conditions

@actuarysailor actuarysailor marked this pull request as draft June 1, 2024 22:41
@actuarysailor
Copy link
Contributor Author

actuarysailor commented Jun 1, 2024

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

@actuarysailor actuarysailor marked this pull request as ready for review June 1, 2024 23:23
@@ -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:
Copy link
Contributor Author

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:
Copy link
Contributor Author

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)
Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

@actuarysailor actuarysailor left a 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.

@actuarysailor
Copy link
Contributor Author

@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 not really, the issue is actually from repo permission differences due to token differences.

@andrewthetechie andrewthetechie merged commit 1bd6d38 into andrewthetechie:main Jun 2, 2024
8 checks passed
@andrewthetechie
Copy link
Owner

andrewthetechie commented Jun 2, 2024

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

@actuarysailor
Copy link
Contributor Author

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

@actuarysailor
Copy link
Contributor Author

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.

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