-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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(dev-infra): add script for merging pull requests [patch version] #37184
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devversion
added
action: merge
The PR is ready for merge by the caretaker
PR target: patch-only
area: dev-infra
Issues related to Angular's own dev infra (build, test, CI, releasing)
labels
May 18, 2020
josephperrott
requested review from
josephperrott
and removed request for
gkalpak
May 18, 2020 18:31
josephperrott
approved these changes
May 18, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Moves the merge script from the components repository over to the shared dev-infra package. The merge script has been orginally built for all Angular repositories, but we just kept it in the components repo temporarily to test it. Since everything went well on the components side, we now move the script over and integrate it into the dev-infra package.
Integrates the merge script into the `ng-dev` CLI. The goal is that caretakers can run the same command across repositories to merge a pull request. The command is as followed: `yarn ng-dev pr merge <number>`.
Sets up the dev-infa merge script in the framework ng-dev configuration file. This allow us to use the script in the future.
The components repo does not use the autosquash merge strategy, so recent changes to that seem to broke the autosquash strategy. Since we don't run the rebase in interactive mode, the `--autosquash` flag has no effect. This is by design in Git. We can make it work by setting the git sequence editor to `true` so that the rebase seems like an interactive one to Git, while it isn't one for the user. This matches conceptually with the merge script currently used in framework. The only difference is that we allow a real interactive rebase if the `commit message fixup` label is applied. This allows commit message modifications (and others) if needed.
As mentioned in the previous commit, the autosquash strategy has not been used in the components repo, so we could easily regress. After thorough manual testing of the autosquash strategy again, now that the merge script will be moved to framework, it came to mind that there is a bug with the base revision in the autosquash merge strategy. The problem is that the base revision of a given PR is relying on the amount of commits in a PR. This is prone to error because the amount of commits could easily change in the autosquash merge strategy, because fixup or squash commits will be collapsed. Basically invalidating the base revision. To fix this, we fixate the base revision by determining the actual SHA. This one is guaranteed to not change after the autosquash rebase. The current merge script in framework fixates the revision by creating a separate branch, but there is no benefit in that, compared to just using an explicit SHA that doesn't need to be cleaned up..
Fix comment typo
devversion
force-pushed
the
merge-script-patch
branch
from
May 18, 2020 18:35
1e177f4
to
1822889
Compare
kara
pushed a commit
that referenced
this pull request
May 18, 2020
Moves the merge script from the components repository over to the shared dev-infra package. The merge script has been orginally built for all Angular repositories, but we just kept it in the components repo temporarily to test it. Since everything went well on the components side, we now move the script over and integrate it into the dev-infra package. PR Close #37184
kara
pushed a commit
that referenced
this pull request
May 18, 2020
Integrates the merge script into the `ng-dev` CLI. The goal is that caretakers can run the same command across repositories to merge a pull request. The command is as followed: `yarn ng-dev pr merge <number>`. PR Close #37184
kara
pushed a commit
that referenced
this pull request
May 18, 2020
Sets up the dev-infa merge script in the framework ng-dev configuration file. This allow us to use the script in the future. PR Close #37184
kara
pushed a commit
that referenced
this pull request
May 18, 2020
The components repo does not use the autosquash merge strategy, so recent changes to that seem to broke the autosquash strategy. Since we don't run the rebase in interactive mode, the `--autosquash` flag has no effect. This is by design in Git. We can make it work by setting the git sequence editor to `true` so that the rebase seems like an interactive one to Git, while it isn't one for the user. This matches conceptually with the merge script currently used in framework. The only difference is that we allow a real interactive rebase if the `commit message fixup` label is applied. This allows commit message modifications (and others) if needed. PR Close #37184
kara
pushed a commit
that referenced
this pull request
May 18, 2020
…37184) As mentioned in the previous commit, the autosquash strategy has not been used in the components repo, so we could easily regress. After thorough manual testing of the autosquash strategy again, now that the merge script will be moved to framework, it came to mind that there is a bug with the base revision in the autosquash merge strategy. The problem is that the base revision of a given PR is relying on the amount of commits in a PR. This is prone to error because the amount of commits could easily change in the autosquash merge strategy, because fixup or squash commits will be collapsed. Basically invalidating the base revision. To fix this, we fixate the base revision by determining the actual SHA. This one is guaranteed to not change after the autosquash rebase. The current merge script in framework fixates the revision by creating a separate branch, but there is no benefit in that, compared to just using an explicit SHA that doesn't need to be cleaned up.. PR Close #37184
Closed in ba9e63a |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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
area: dev-infra
Issues related to Angular's own dev infra (build, test, CI, releasing)
cla: yes
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Patch version of #37138. Only difference is that the
package.json
had to be slightly adjusted for the patch branch.