Skip to content

Conversation

@MIchaelMainer
Copy link
Contributor

@MIchaelMainer MIchaelMainer commented Apr 24, 2023

This PR contains changes to organization policy that will result in requiring:

  • a minimum of 1 reviewer for a PR into main, master, dev, and [Rr]elease/* branch specs for SDKs, apps, services, and tools.
  • secret scanning, secret scanning push protection, dependency update checks, and blocking PRs with Moderate or higher alert levels for all PRs in all repositories in the organization.
  • SECURITY, LICENSE, CODE_OF_CONDUCT, and CONTRIBUTING files for all public repositories in the organization. These lists will need to be periodically updated.

Future work in support of policy updates:

CLI command to get all public repositories

gh repo list microsoftgraph --no-archived --visibility public -L 300 --json name --jq '.[].name' | sort
  • All repositories must have CODEOWNERS set before merging this PR.

@MIchaelMainer
Copy link
Contributor Author

@jeffwilcox Does the "Remove from repositories" for the branch protection rules mean that the rules set in this PR will not apply to that count of repos? Also, does it override all settings found in the organization settings UI?

@MIchaelMainer MIchaelMainer marked this pull request as ready for review April 25, 2023 16:24
@jeffwilcox
Copy link
Contributor

@MIchaelMainer correct on the remove from, however, I think it may be a side effect of how you're using multiple policy files to apply differently. would be a good question for that team to confirm.

Once you choose to use the policy enforcement mechanism this all overrides all UI choice - so you cannot use the UI any more - so it will be a learning experience to use this.

@gavinbarron
Copy link
Member

This is a big step forward to centralize our branch protection and policy work.

@jeffwilcox if I'm understanding you correctly, the centralized policy replaces any previously configured policies on the individual repos?

I'm curious how this interacts with existing branch protection rules requiring specific workflows to run, e.g. our CI workflows. Do they now become optional? Will they trigger? Can we enforce that CI builds pass before a PR can be merged?

@jeffwilcox
Copy link
Contributor

@gavinbarron I'm not best to answer your questions on the specifics - we aren't using these policies anywhere else in our open source presence today, so I think if anything this is a bit of a pilot.

Any existing configuration is likely overwritten by this.

It might make sense to test some of this somehow with a smaller applicability filter?

@JohannesLampel
Copy link
Collaborator

@gavinbarron yes, the policy will delete existing rules for branches and overwrite them with the rules specified in the policy-file.
This means that the checks required will become optional. They should still be triggered by your GitHub action though.
You can specify which checks to enforce using requiredStatusChecks in the policy.

@MIchaelMainer yes, the remove from is triggered by the filters you set up. It will essentially exclude these repos when enabling the specified policy.

@markphip
Copy link

This is a big step forward to centralize our branch protection and policy work.

@jeffwilcox if I'm understanding you correctly, the centralized policy replaces any previously configured policies on the individual repos?

I'm curious how this interacts with existing branch protection rules requiring specific workflows to run, e.g. our CI workflows. Do they now become optional? Will they trigger? Can we enforce that CI builds pass before a PR can be merged?

Once you put the policy in place, the rules in the policy will have total control and undo any changes you attempt to make to the branch protection rules via the UI. You can add policies at the repository level to add additional rules to what is specified at the organization level. For example, the requiredStatusChecks might often be something you add to the org policy at the repo level as the specific status checks are often unique to the repository.

In general, be careful with the requiredStatusChecks setting. Since you cannot edit the branch protection rules using the UI, you can create a situation where you commit a policy that requires a check that does not exist and this prevents you from being able to change the policy. As long as you allow a user with Admin to still merge a PR without the checks you will be alright, but if you combine those features you can paint yourself into a corner.

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Great to see this moving forward, thanks for taking care of it!

# Require status checks to pass before merging. TODO: this value should be true, we should work to support this.
# Used with the requiredStatusChecks setting to specify which checks must pass for the PR to be merged.
requiresStrictStatusChecks: false
# TODO: all commits should be signed. We need to get everyone signing their commits.
Copy link
Member

Choose a reason for hiding this comment

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

let me know if you want me to drop my setup script somewhere in a doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you provide me with a link to your script so I can add it to my planning doc?

Copy link
Member

Choose a reason for hiding this comment

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

#this line is only for windows, we'll need to document the equivalent for linux and mac
winget install GnuPG.Gpg4win
#generate a certificate, the email must match for the line below, upload the public key to github sign keys, get the certificate thumbprint. For all those steps, see the blog post below
git config user.email <microsoft-email>
git config user.signingkey <certificateThumbprint>
git config user.name <FirstNameLastName>
git config commit.gpgsign true
git config tag.gpgsign true
# this line is only needed for windows
git config --global gpg.program "C:\Program Files (x86)\GnuPG\bin\gpg.exe"
# for linux/mac if the $PATH is setup to include gpg, it should not be needed

Note: add --global to configure for all repos

https://jamesmckay.net/2016/02/signing-git-commits-with-gpg-on-windows/

@MIchaelMainer
Copy link
Contributor Author

Thank you everyone for this very helpful discussion. It is now very clear that requiredStatusChecks should probably not be set at the org-level unless there is a very broad scenario across all of our repos we should check.

Due to the potential for disruption, @gavinbarron and myself will set aside sometime on Monday to do a pilot change to the org-policy for the https://github.com/microsoftgraph/microsoft-graph-toolkit repository. We will apply all of the changes above to just this repository. We just need to confirm whether we can use the where clause on all policy file types.

@markphip @JohannesLampel Is the where clause supported for all policy file types? We are specifically interested in adding it for the advancedsecurity policy file.

@JohannesLampel
Copy link
Collaborator

@MIchaelMainer yes it is.

@MIchaelMainer
Copy link
Contributor Author

We are putting this PR on hold until Gavin and I validate the changes on just MGT.

#10

requiredApprovingReviewsCount:
min: 1
# Must have a CODEOWNER approve for the PR to be merged.
requireCodeOwnersReview: true

Choose a reason for hiding this comment

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

With just one owner, we have a situation where the code owner is away on leave for a considerably long period of time, and there are no secondary owners. Would this be a challenge that slows down work output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Shem for bringing this up. We should always have secondary owners setup. We should pull in this change until we have GitHub team setup so we can easily add new CODEOWNERS without requiring PRs. I'll make sure I get your review on the GitHub teams management document so that we get this addressed.

@microsoft-github-policy-service

7 policies from organization microsoftgraph will be applied after PR merge.

Enterprise Policy File Apply to repositories Exclude from repositories
Organization Policy File Apply to repositories Exclude from repositories
policies/advancedsecurity.yml 253 0
policies/branch-protection-apps-services.yml 15 238
policies/branch-protection-sdks.yml 43 210
policies/cla.yml 253 0
policies/mandatory-files-public.yml 113 140
policies/mandatory-files.yml 253 0
policies/platformcontext.yml 253 0

Total execution time: 38.51 seconds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.