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: add commit message scope for migration changes #36390

Conversation

devversion
Copy link
Member

@devversion devversion commented Apr 2, 2020

This is a proposal commit that adds a separate scope for migration changes.

The motivation is that migrations aren't necessarily always affecting @angular/core
(e.g. the move-document migration is affecting platform-browser). Migrations are just
stored in the @angular/core package in favor of a canonical location when someone
runs ng update. Additionally, it rather seems confusing that migration changes are listed
under core in the changelog. Long commit message titles are needed to make clear that
commits do not affect @angular/core runtime logic, but just the ng update migrations.

@devversion devversion added area: build & ci Related the build and CI infrastructure of the project action: discuss action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Apr 2, 2020
@ngbot ngbot bot added this to the needsTriage milestone Apr 2, 2020
@gkalpak
Copy link
Member

gkalpak commented Apr 2, 2020

OOC, why not use the proper scope (instead of core) in the commit messages (even if the changes are in packages/core/)?

@devversion
Copy link
Member Author

devversion commented Apr 2, 2020

That's a good point. I think it would be still ambiguous if we would use e.g. platform-browser if the migration affects the platform-browser package as the actual changes are made in packages/core/.

Additionally it would still require longer commit messages, and in the changelog, migration changes would not be grouped. I guess it would be good to have migrations show up separately in the changelog as they don't affect core runtime issues. Similarly to the ngcc scope that could be just compiler-cli too.

@devversion
Copy link
Member Author

@IgorMinar Would you be able to provide some feedback on this? It's not urgent though 😄

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

SGTM but we should announce this change and in order to enforce it we will need reviewers to look for the current scope to be applied. Changing the pullapprove settings to create a new group for migrations would make that easier.

@devversion
Copy link
Member Author

devversion commented Jun 10, 2020

@IgorMinar Thanks for reviewing! Yeah, we added a separate code owner group for migrations already (see here).

Definitely agreed that we should announce this. I'm wondering if we could do better at enforcing these commit scopes. That might be a good thing to look into from the dev-infra side.

This is a proposal commit that adds a separate scope for
migration changes. The motiviation is that migrations aren't
necessarily always affecting `@angular/core`, but are just
stored in the core package for a canonical location when
someone runs `ng update`. Additionally, it rather seems confusing in the
changelog if migration changes are listed under `core`.
@devversion devversion force-pushed the build/propose-migrations-commit-msg-scope branch from aeecf21 to 0cb37c9 Compare June 10, 2020 09:24
@devversion devversion removed the request for review from josephperrott June 10, 2020 09:26
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 10, 2020
@atscott atscott closed this in bd9c1e6 Jun 10, 2020
atscott pushed a commit that referenced this pull request Jun 10, 2020
This is a proposal commit that adds a separate scope for
migration changes. The motiviation is that migrations aren't
necessarily always affecting `@angular/core`, but are just
stored in the core package for a canonical location when
someone runs `ng update`. Additionally, it rather seems confusing in the
changelog if migration changes are listed under `core`.

PR Close #36390
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
This is a proposal commit that adds a separate scope for
migration changes. The motiviation is that migrations aren't
necessarily always affecting `@angular/core`, but are just
stored in the core package for a canonical location when
someone runs `ng update`. Additionally, it rather seems confusing in the
changelog if migration changes are listed under `core`.

PR Close angular#36390
@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 Jul 11, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
This is a proposal commit that adds a separate scope for
migration changes. The motiviation is that migrations aren't
necessarily always affecting `@angular/core`, but are just
stored in the core package for a canonical location when
someone runs `ng update`. Additionally, it rather seems confusing in the
changelog if migration changes are listed under `core`.

PR Close angular#36390
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: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants