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

Add sync support for dev branches and tune PR description #15

Merged
merged 3 commits into from
Jan 4, 2022

Conversation

dagood
Copy link
Member

@dagood dagood commented Dec 16, 2021

This PR adds automatic merges from microsoft/release-branch.go1.17 into dev/official/go1.17-openssl-fips.

I touched up the PR description a bit. The main change was to truncate the diff after 200 lines to avoid potential issues with unbounded output. (See code comment.) I went after this right now because with dev branches, the diff could get very large depending on what's being worked on. While I was there, added a couple more things:

  • A link to a doc that points at the sync config and entrypoint.
  • A collapsible section for the file diff, because usually when you look at one of these PRs, it's because it failed, and this diff isn't useful.

Example PR: dagood/go#9

@dagood dagood marked this pull request as ready for review December 16, 2021 20:14
@dagood dagood requested a review from a team as a code owner December 16, 2021 20:14
cmd/sync/sync.go Outdated
@@ -126,6 +131,21 @@ func main() {
}
}

// createAuthorizedGitUrl takes a URL, auth options, and returns an authorized URL. The authorized
// URL may be the same as the original URL, depending on the options given and the URL content.
func createAuthorizedGitUrl(url string, gitAuthSSH bool, gitAuthPAT bool) string {
Copy link

Choose a reason for hiding this comment

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

Nit: it feels weird to be allowed to pass two option flags, but have one of them ignored if the other is true. Does this ever get called with both false? Feels like it might be better to have just a single 'usSSH' and then fall back to PAT if not true?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess currently it wouldn't happen because our fork repos are private and need auth, but false, false would be for running a sync locally and not actually pushing a PR anywhere. (The Go upstream repos don't require any auth for fetching.) But yeah, this is not a good API--there are only three "real" states for four input values. Should really be an enum or similar.

I'll try changing the bool command line options -git-auth-ssh , -git-auth-pat to -git-auth ssh, -git-auth pat to drive this change all the way through.

Copy link
Member Author

Choose a reason for hiding this comment

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

This func now takes a GitAuthOption (just a typed string) and compares against typed consts in a switch. Help looks like this:

$ go run ./cmd/sync -git-auth wrong
Error: git-auth value 'wrong' is not an accepted value.

[...]
  -git-auth string
        The type of Git auth to inject into upstream and target GitHub URLs for fetch/push access. String options:
         none - Leave GitHub URLs as they are. Git may use HTTPS authentication in this case.
         ssh - Change the GitHub URL to SSH format.
         pat - Add the 'github-user' and 'github-pat' values into the URL.
         (default "none")
  -github-pat string
        Submit the PR with this GitHub PAT, if specified.
[...]

@dagood dagood merged commit aad072f into microsoft:main Jan 4, 2022
@dagood dagood deleted the dev/dagood/fips-branch-sync branch January 4, 2022 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants