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

ci(settings): added initial settings that should be applied consistently across the org #9

Merged
merged 2 commits into from
Dec 4, 2020

Conversation

travi
Copy link
Member

@travi travi commented Dec 1, 2020

for #7

i think this could be merged as is, but it does have a few gaps that we will probably want to close with follow up changes:

  • it does not define any labels because i would want to be very careful with what we define, if any. if the list does not include all of the labels already used in the repository, the ones that arent included will be removed from the repository (even any existing issues/PRs that use them)
  • it only grants permission for the maintainers team. i still wonder if there would be value in an admins or owners team, or maybe a triage or contributors team that we could define other permission levels for. then we could add other members to those teams in the future and have the permissions already granted. we can discuss and update later if we decide to add anything like that

… directory

especially for the `settings.yml` since it has the ability to modify the admin settings of the
repository
@travi travi changed the title WIP: ci(settings): added initial settings that should be applied consistently across the org ci(settings): added initial settings that should be applied consistently across the org Dec 1, 2020
@travi travi requested a review from gr2m December 1, 2020 03:19
@travi
Copy link
Member Author

travi commented Dec 1, 2020

i enabled the settings app for all repos in the org, so this will be applied to this repo upon merging and available for extension from the others

Copy link
Member

@gr2m gr2m 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, just wondering if we should expand branch protection to support the release workflow?

has_projects: false
allow_squash_merge: true
allow_merge_commit: false
allow_rebase_merge: false
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

@@ -0,0 +1 @@
/.github/** @semantic-release/maintainers
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

dismissal_restrictions: null
required_status_checks:
strict: false
contexts: ['test', 'WIP']
Copy link
Member

Choose a reason for hiding this comment

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

should we also add branch protection for the pre- and maintenance releases?

  • next
  • beta
  • *.x

Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately, i believe the api is limited from setting up protection on branches that dont exist. i just confirmed with travi-test/settings-test@17cba6c that its not working. if you happen to have insight to suggest this should work, I would love to be able to support this.

do we currently define this type of protection? the other thing to consider, if so is that it might get removed if we dont define it

Copy link
Member Author

Choose a reason for hiding this comment

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

dug a little deeper to at least confirm that the current issue is a 404 result with message "Branch not found" for

method: "PUT"
url: "https://api.github.com/repos/travi-test/settings-test/branches/alpha/protection"

Copy link
Member Author

Choose a reason for hiding this comment

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

it looks like we at least wont remove existing protection rules when not defined in the file

Copy link
Member

Choose a reason for hiding this comment

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

good to know, I thought I had branch protection for all release branches implemented before, but I only created the issue as a reminder. Thanks for looking into this.

All good then, let's ship!

Copy link
Member Author

Choose a reason for hiding this comment

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

looking back through settings issues, i did find this mention that it should work: repository-settings/app#136 (comment). my tests, even last night, through octokit rest suggest otherwise, but i havent dug much deeper than our current implementation and the octokit rest docs.

i dont find a mention of createBranchProtectionRule directly in the octokit docs, so i would have to follow up to better understand. are you familiar with an approach other than what is currently happening to adjust with octokit rest? i'm also open to going down the graphql route, but not sure where to start without some digging.


teams:
- name: maintainers
permission: maintain
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

@travi travi merged commit 441c741 into main Dec 4, 2020
@travi travi deleted the settings branch December 4, 2020 16:30
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