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

build: create merge script w/ cherry-picking #18856

Merged
merged 2 commits into from
Apr 22, 2020

Conversation

devversion
Copy link
Member

Creates a merge script that automatically merges pull
requests from Github and cherry-picks them into the
right publish branches based on the PR target label.

The script has been designed in an extensible and programmatically
acessible way, so that it can be used in other Angular repositories too.

The script provides infrastructure for PR merge safety checks. e.g.
it currently enforces that the CLA is signed, or that the merge ready
label is assigned, or that no CI checks are failing / pending.

@devversion devversion added merge safe target: patch This PR is targeted for the next patch release labels Mar 18, 2020
@devversion devversion requested review from jelbourn and a team as code owners March 18, 2020 15:43
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 18, 2020
@devversion devversion force-pushed the wip/merge-script branch 2 times, most recently from c88180e to cb4b3dc Compare March 18, 2020 16:06
merge-config.js Show resolved Hide resolved
scripts/merge-script/cli.ts Outdated Show resolved Hide resolved
scripts/merge-script/cli.ts Outdated Show resolved Hide resolved
scripts/merge-script/cli.ts Outdated Show resolved Hide resolved
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

Overall, I think its in good shape, but just a few bigger questions to answer with the first one being are we okay with not having the PR marked as merged in the Github UI.

A couple higher level comments as well:

  • I don't know if its worth having the script support multiple merges with one command. We want to ensure we have a CI run for each PR that is merged so we would need to push multiple times anyway.

  • Some of these files are pretty lengthy, I am not sure if there is a best option, but I would be in favor of splitting some of the reusable pieces into other files if there are logical places to do so.

scripts/merge-script/index.ts Outdated Show resolved Hide resolved
scripts/merge-script/index.ts Outdated Show resolved Hide resolved
scripts/merge-script/index.ts Outdated Show resolved Hide resolved
scripts/merge-script/cli.ts Outdated Show resolved Hide resolved
scripts/merge-script/index.ts Outdated Show resolved Hide resolved
scripts/merge-script/index.ts Outdated Show resolved Hide resolved
scripts/merge-script/index.ts Outdated Show resolved Hide resolved
scripts/merge-script/commit-message-filter.js Outdated Show resolved Hide resolved
scripts/merge-script/config.ts Show resolved Hide resolved
scripts/merge-script/console.ts Outdated Show resolved Hide resolved
scripts/merge-script/index.ts Outdated Show resolved Hide resolved
@devversion
Copy link
Member Author

@jelbourn Addressed your feedback (and also simplified things a bit). Also, are you aware of the fact that this script doesn't preserve the purple Merged state for pull requests? I investigated whether we can merge purely with the Github API, but that's not possible. Additionally, we can't merge using the API, and then cherry-pick into other branches as that would mean that autosquashing doesn't work. Autosquashing is commonly used in framework.

I think we should just acknowledge that this is a Github limitation. Github should just provide an API for changing the PR state to merged. Clearly merges using the Squash & Merge button aren't fast-forward either.. so Github is already tricking here.

@jelbourn
Copy link
Member

Ah, if we can't preserve the purple "merged" status, then I would prefer to continue pressing the button in the UI. Maybe then the script could handle the cherry-picking after merging the PR.

@devversion
Copy link
Member Author

devversion commented Mar 31, 2020

@jelbourn I think the problem here is that this script should be also usable in other repositories. If it would be only used in the components repository, then we could definitely use the Github PR merge API, and perform the cherry-picking locally.

Though I wanted the script to be part of the shared dev-infra tools, so that it can be used in framework too. Framework relies on auto-squashing which isn't supported by the Github merge buttons. We don't perform fast-forward merges (we cannot), so if we ever want that behavior, Github needs to provide an API for us to set the merged state.

Alternatively we can add opt-in options to the script that allow:

  • Squash merging through pressing the button in the UI. The script only does the cherry-picking. This would also simplify the process of doing manual commit message cleanup.
  • Squash merging through the API (automatically). The script does the cherry-picking as needed.

Basically, the script would be made "suitable" for requirements from both repositories where the script is used. I personally would prefer having a single canonical way, but I understand the reason for having it show up as Merged. At the same time though.. I'd like to start using fixup! commits in the components repo, and not having to request for preserving of multiple commits (if needed).

Also only cherry-picking a merged PR through the Github API/UI will also complicate things, as it will be difficult for the script to find corresponding PR commits if the PR has been merged through the Rebase and Merge button.

Another interesting alternative is that the PR can merged through API into a temporary _pull-requests branch. Just for the sake of showing the purple merged state?

@devversion
Copy link
Member Author

@jelbourn @josephperrott I made the changes we discussed in our meeting about this. The merge strategy for using the Github API turned a bit more code than initially anticipated, but I think that amount of code is reasonable and acceptable. Easy to remove if we ever want a canonical merge strategy across all repositories.

Copy link
Member

@jelbourn jelbourn 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 overall, just a few small comments

merge-config.js Outdated Show resolved Hide resolved
scripts/merge-script/cli.ts Outdated Show resolved Hide resolved
scripts/merge-script/cli.ts Show resolved Hide resolved
scripts/merge-script/pull-request.ts Outdated Show resolved Hide resolved
scripts/merge-script/config.ts Outdated Show resolved Hide resolved
scripts/merge-script/index.ts Outdated Show resolved Hide resolved
scripts/merge-script/index.ts Outdated Show resolved Hide resolved
scripts/merge-script/pull-request.ts Outdated Show resolved Hide resolved
scripts/merge-script/pull-request.ts Outdated Show resolved Hide resolved
scripts/merge-script/strategies/api-merge.ts Outdated Show resolved Hide resolved
scripts/merge-script/strategies/api-merge.ts Outdated Show resolved Hide resolved
scripts/merge-script/strategies/api-merge.ts Show resolved Hide resolved
message: 'Please update the commit message',
default: commitMessage,
});

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 be validating the commit message that is created here?

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'd say yes, but that seems to be something we can tackle as a follow-up? Especially since we are not having commit message validation in the components repo yet. I'd like to set it up soon, but I think we need some upstream changes to account for a different scope that we support.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

scripts/merge-script/strategies/strategy.ts Show resolved Hide resolved
Creates a merge script that automatically merges pull
requests from Github and cherry-picks them into the
right publish branches based on the PR target label.

The script has been designed in an extensible and programmatically
acessible way, so that it can be used in other Angular repositories too.

The script provides infrastructure for PR merge safety checks. e.g.
it currently enforces that the CLA is signed, or that the merge ready
label is assigned, or that no CI checks are failing / pending.
@devversion
Copy link
Member Author

@jelbourn @josephperrott Addressed most of the feedback. Please have another look.

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

scripts/merge-script/pull-request.ts Outdated Show resolved Hide resolved
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Apr 22, 2020
@jelbourn jelbourn merged commit 9d31d71 into angular:master Apr 22, 2020
jelbourn pushed a commit that referenced this pull request Apr 22, 2020
* build: create merge script w/ cherry-picking

Creates a merge script that automatically merges pull
requests from Github and cherry-picks them into the
right publish branches based on the PR target label.

The script has been designed in an extensible and programmatically
acessible way, so that it can be used in other Angular repositories too.

The script provides infrastructure for PR merge safety checks. e.g.
it currently enforces that the CLA is signed, or that the merge ready
label is assigned, or that no CI checks are failing / pending.

* fixup! build: create merge script w/ cherry-picking

Address feedback
soro-google pushed a commit to soro-google/components that referenced this pull request Apr 24, 2020
* build: create merge script w/ cherry-picking

Creates a merge script that automatically merges pull
requests from Github and cherry-picks them into the
right publish branches based on the PR target label.

The script has been designed in an extensible and programmatically
acessible way, so that it can be used in other Angular repositories too.

The script provides infrastructure for PR merge safety checks. e.g.
it currently enforces that the CLA is signed, or that the merge ready
label is assigned, or that no CI checks are failing / pending.

* fixup! build: create merge script w/ cherry-picking

Address feedback
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants