Update to Angular v9 automatically checks in the result!

[DeborahK]

🐞 Bug report

Command (mark with an x)

- [ ] new
- [ ] build
- [ ] serve
- [ ] test
- [ ] e2e
- [ ] generate
- [ ] add
- [x ] update
- [ ] lint
- [ ] xi18n
- [ ] run
- [ ] config
- [ ] help
- [ ] version
- [ ] doc

Is this a regression?

Yes, the previous version in which this bug was not present was: ....

Description

When running: `ng update @angular/cli @angular/core --next --allow-dirty`

It automatically checked in the result to my git repo. This is definitely NOT desired. In many use cases, this may be a "Trial" without a desire to check in. In other use cases, this may be a key dev environment that the developers would want to try out before committing.

In any case, this should NOT automatically check in without at least asking first.

UPDATE: In looking into this further, it does THREE commits during the update process.

🔬 Minimal Reproduction

1. Run: ng update @angular/cli @angular/core --next --allow-dirty

2. Check the Git change list, all changes are check in.

🔥 Exception or Error

No error

🌍 Your Environment

Was 8.1.3 prior to the update.

Now: 
Angular CLI: 9.0.0-rc.0
Node: 10.16.0
OS: win32 x64
Angular: 9.0.0-rc.0
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.0-rc.0
@angular-devkit/build-angular     0.900.0-rc.0
@angular-devkit/build-optimizer   0.900.0-rc.0
@angular-devkit/build-webpack     0.900.0-rc.0
@angular-devkit/core              9.0.0-rc.0
@angular-devkit/schematics        9.0.0-rc.0
@ngtools/webpack                  9.0.0-rc.0
@schematics/angular               9.0.0-rc.0
@schematics/update                0.900.0-rc.0
rxjs                              6.5.3
typescript                        3.6.4
webpack                           4.41.2

Anything else relevant?

@vikram @StephenFluin

[filipesilva]

This is especially tough because users that are not very proficient with git will have a hard time removing these commits.

[clydin]

This behavior can be disabled via the --skip-commits/-C option.

I believe the concept here was that an update is similar to the development of a PR for a feature or bug fix. A user would create a branch and then make a set of changes. After completion, adjustments would be made to ensure everything is working and then the results would be squashed/rebased and then pushed upstream for review and eventual merge.

The commit per migration also has the advantage of allowing the changes from each migration to be viewed in isolation and altered with a higher degree of confidence, if needed. Explanations for the migrations are also contained within the corresponding commit messages. Some migrations can result in hundreds or more small changes across the entire codebase.

cc @IgorMinar

[filipesilva]

We don't tell users to make a branch in the update guide though, I think.

I also wonder how these commits interact with projects that have git hooks for the commit messages. One of the projects I'm testing updates on uses husky for that.

[DeborahK]

This may work well for some users and use cases ... but not all developers using Angular are well experienced with git or have other standard processes and procedures.

Plus with the automatic check-in, it is much more difficult to see which files that the update modified.

Also, since it does at least 3 check ins, it makes it even harder for a developer to undo this.

We should NOT be doing this without explicit permission. Make the flag --skip commits true by default and/or ask a question. Then those devs with experience and who work the way you are expecting can CHOSE to allow automatic check ins.

Thanks!

[clydin]

That was just an example use case; although one that probably should be considered a recommended practice.

As to git hooks, they intentionally don't interact: git commit --no-verify -m "${message}"

[filipesilva]

Related to #14600

[clydin]

I agree this can cause issues with those inexperienced with git and it is something that should be considered. That linked issue, however, does provide an additional use case for the current behavior. Performing an update on top of an unknown quantity of uncommitted changes can have catastrophic results to a project. Although, the linked situation could be mitigated by adding an additional message instructing the user to verify and commit the results of the update operation.

As an aside and for anyone curious, the command to remove the three last commits and keep the changes is git reset HEAD~3. To remove the commits and the associated changes: git reset --hard HEAD~3

[Splaktar]

I agree that this is certainly a surprising, even startling, change for many Angular CLI users.

At minimum, if this is the default behavior, there need to be workspace options in angular.json to enable both --allow-dirty and --skip-commits for users who want the previous behavior.

I've tried a number of updates now across different apps with this behavior in place and I can tell you that it was never desirable in any of those cases.

• The commit message needed to be reworded (not trivial for many Git users)
• The commit needed to be amended to add fixes or changes so that tests would pass and the build would work. The repo doesn't want commits that don't build/pass tests.
• The commits needed to be squashed in some cases (not trivial for many Git users)

I've had to use the --allow-dirty flag manually from the CLI almost one hundred times now and every time I wish that I could just set a flag to make that the default behavior.

In my experience doing updates to real world apps (which is significant), I've run into the above issues many times with this new behavior. I've never run into a case where I had a dirty branch with lots of unrelated changes and ended up with a catastrophic result.

I am perfectly happy with a prompt or warning that costs me one extra key press during ng update to verify that my current branch is in a good state.

[filipesilva]

Although --allow-dirty is related to the commit functionality, I don't think it should be conflated with the auto-commit facility as originally listed in this issue.

[gnomeontherun]

Perhaps a middle ground solution would be to ask after each update step (package changes, migration script) that makes a change to inform the user about the changes and ask if they wish to commit or continue?

The challenge of not committing is that its very easy for changes to happen by the user that break the migration, and then its hard to track what happened. Git history can be a big help here, but as others have said it depends on one's familiarity with git. When 100s of files are touched automatically, I need confidence where those changes came from, me or the update process.

Also, there can be failure message reported far up in the migration result logs that are easily lost by someone if they see it ends with success at the end. If each step was more individual with confirmation, I would feel more confident in understanding what happened.

[Splaktar]

The name --skip-commits is hard to associate with my desire to disable automatic committing of migration changes during ng update. It would expect something like ng update --auto-commit or ng update --auto-commit=false.

[DeborahK]

I suggest we make this a CHOICE:

• Off by default
• The options for automatic check in more clearly defined in the update instructions so those that want it can get the auto check ins by setting a flag.

As it is, the developer does not have an opportunity to see what is changed or easily undo the changes (minimum of THREE check ins) especially if they are not git experts.

If we do need to keep this feature ... I suggest we add LARGE warnings everywhere that this auto check in will happen and that developers cannot just "try out" an update.

[wardbell]

Wow. All that’s missing is to skip the PR (why bother?) and push the branch (master of course). What could be easier? 😉

I am wary of automatic commits. I am (mostly) unpersuaded by the arguments that I've read in favor. The following is a list of quotes (for and against) and my commentary from a slack channel thread.

“It means that the exact changes made are in history now and I don’t accidentally make other changes and lose track of them in the mess.”

Not convinced: The changes are in the working set (or maybe staged) so there is no history to lose. If I make more changes during the upgrade that is almost certainly because I had to make those changes as part of the update.

“There are a lot of migration steps that might run, it would be nice if those were individually committed.”

Yes but will I be able to make sense of the intermediate commits? Are they distinct in a way that is meaningful to the developer? Could I undo any of them? Reorder? Squash? I don’t really know why one commit or three and I am not sure why I should care.

“there are errors that weren’t fixed automatically but they get lost in the stream of logs and easily missed.”

Is that an argument FOR auto-committing? If an upgrade fails, after 2 previous commits, what is my recourse? Do I fix and restart? How? How do I roll back? Assume I’m a git tenderfoot who neglected to start a new branch (still in master) before upgrading.

TBH, I am a person who has been trapped mid-update. When that happens, all I want to do is cry, rollback, and post a message on a slack channel.

“The commit per migration [allows] changes from each migration to be viewed in isolation and altered with a higher degree of confidence, if needed. Explanations for the migrations are also contained within the corresponding commit messages.”

This argument has some merit … although when you add that there can be “hundreds or more small changes across the entire codebase”, I think you’ve undermined that argument.

The commit message is boilerplate. Could emit the combined messages to console or to a dated v9_conversion.readme.txt file

“I personally would like to check the changes once again before it gets committed.”

I would like to do that too. While many people have tooling that makes it somewhat easy to review the changes wrought by recent commits, it sure is nice to see them in VS Code as pending changes.

"This behavior can be disabled via the --skip-commits/-C option."

Seriously? Who knows about that … or will remember to add it?

“This is especially tough because users that are not very proficient with git will have a hard time removing these commits.”

I agree strongly. At least we should provide a mechanism to revert (perhaps display a git reset SHA --hard advice in the console when it’s over.

Committing feels too paternalistic. If I know how to use git, you have kind of offended me. If I’m new to git, you’ve set me up for a potentially nasty fall as I do what the console invites me to do: npm push.

The slack thread had more arguments in favor but I think I've addressed the strongest of them.

Reasonable minds can (and do) differ on the value of auto-committing.

I’d like to have the choice: (no commit by default) and have the benefits of auto-commit explained … along with a stern warning against the git push reflex … both BEFORE and AFTER.

[juristr]

Just my 2 cents as well.

When I saw the autocommit during the upgrade I was surprised. For me personally it is not that much of a big deal, as I'm doing the upgrade in a separate branch anyway and I used to always create intermediate commits during the upgrade (like after upgrading core and cli, and then after material etc).
That said, I would never leave those commits as is, as they do not comply with my conventional commit messages. So I'd rather squash/rebase whatever those commits before merging them in.

But, I see this a bit problematic for the "average developer" where - unfortunately - I still see a lot of problems related to everyday Git usage (that's why I created a hugely successful Git course on Egghead). So they would be irritated and wouldn't necessarily know how to undo these automatic commits etc.

So

• I'd rather ask users during the upgrade if they want to commit the changes
• provide opt-in flags to pass to the ugprade
• explicitly mention it in some docs

[benelliott]

Just want to point out that the --skip-commits option is new to Angular CLI v9. So for the (typical) case when updating from v8 to v9 the workflow as per the migration guide (https://next.angular.io/guide/updating-to-version-9) looks like this:

1. run npm install --no-save @angular/cli@^8.3.15 && ng update @angular/cli @angular/core --next. Note that trying to use the --skip-commits option here will fail as it doesn't exist in v8
2. ng update runs, updating @angular/cli to version 9. Then the new version (?) of the CLI is invoked to migrate @angular/core to v9, with the --skip-commits option now available. But the user can't really interrupt this migration to tell the CLI not to commit.

The only way around this I could find was to kill the update process at the point where the package.json had been changed but the migrations had not yet been run (ie. no auto-commits yet). Then I explicitly ran the migrations with --migrate-only --skip-commits --allow-dirty.

[clydin]

@benelliott There is no need to attempt to kill the process. All options are passed during the update bootstrap process.
Executing the following will result in no commits being generated:

npm install --no-save @angular/cli@^8.3.15 && ng update @angular/cli @angular/core --next --skip-commits

This was tested with the latest published packages against a CLI 8.3.0 & Framework 8.2.13 project.

[benelliott]

@clydin Sorry about that - not sure what I did wrong initially!

[StephenFluin]

I'm in favor of flipping the default to be skipping the commits and leaving the sum of all changes on disk. I updated a bunch of applications last night, and I believe the most common usecase for users is to run a command that they don't fully understand (Migration can't be applied one by one as I understand it), and then analyze what it did via git status / visual tools.

[fmalcher]

+1 for @DeborahK and the following! When you know there’s a new commit, this feature is more or less okay. But since there are schematics active that could update my code I want to check what they did before commiting this. This could be confusing for people who don’t know that a commit will be made. Of course everything is possible if you know what you're doing, I can undo a commit at any time… but anyway. I don’t see the real value of this change ;-)

[SanderElias]

In addition to the above where I certainly agree with @DeborahK and @wardbell,
and I want to see this as an optional future that is off by default as @StephenFluin does, I want to add the following.

1. this is a breaking change, for all the above reasons.
2. as it does multiple commits, undoing it needs more git knowledge as the average developer has. (not hard, but needs a lookup)
3. with multiple commits it needs multiple steps to undo!
4. breaks expectations from the past

the path forward could be 1 of 2:

1. Make this optional, so the old behavior is the default
2. automatically create a branch for this.

With option 2 you keep most of the benefits from the old way, and the new way.
at the cost of 1 extra developer step (merge in the branch)

[clydin]

Just a quick note on point 3, undo-ing the update no matter the number of commits can be a single command if desired: git reset --hard <SHA before the update>. In that same regard, there is also a proposal for a future addition to provide fallback in the event of an update failure by querying the user and then executing the aforementioned along with a package install if the user answered in the affirmative. (Assuming the user did not use the --allow-dirty option in which case such an action would be dangerous and could erase an untold amount of the users work. The question would most likely not be offered in this case.)

The undo git command could also be offered to the user at the completion (successful or otherwise) regardless of the commit per migration decision outcome as it would be valid in both scenarios.

[SanderElias]

@clydin looking up the <SHA before the update> is a step in itself in my opinion.

[aaronfrost]

I ran the ng update @angular/cli @angular/core command without knowing what to expect. I was going to look at changed files to learn about all the changes. But, if you hiding the changes by committing them, then that doesn't help me understand whats happening.

I too vote for what @StephenFluin is suggesting.

[clydin]

@SanderElias You're right and that is correct. But it would always be two steps regardless of the number of commits. Many IDEs with git integration (highly recommended in general) will also show the git history graphically which can reduce the user actions to only one git command. The outlined proposal above would also completely remedy the need for any lookup which, I agree, could be confusing for someone with limited git experience.

[SanderElias]

@clydin The current version of the CLI doesn't do any commit's. So reverting back to the last known good state is always:

git reset HEAD --hard

And done. 1 step, no second guesses.
Also, I could easily see all the changes to my app in the git diff in my vs-code, without having to do anything.
Now that also takes additional steps.

I really hope this is not set in stone just yet!

[wardbell]

If auto-commit remains the default, can we print near the bottom of the console log

• a notice explain that the upgrade changes have been committed
• the git command that reveals those commits (git log --oneline -n) where n is the exact number of said commits
• advice for rollback containing the exact git command that will reverse those commits (git reset --hard SHA with the actual SHA).

Such actionable information may go far to reduce anxiety.

[DeborahK]

Alternatively, can we meet the requirement of providing migration change information by adding comments in the code? Something like this:

  // MIGRATION: Static flag migration removes the `static` flag from dynamic queries.
  // As of Angular 9, the "static" flag defaults to false and is no longer required for your view and content queries.
  // Read more about this here: https://v9.angular.io/guide/migration-dynamic-flag
  @ViewChild('filterElement') filterElementRef: ElementRef;

The benefits of this are:

• The developer can readily see the change WHERE IT IS WAS CHANGED instead of a (potentially large) set of console logs.
• The developer will more easily see the helpful text (read more about this here...), instead of having to read through the console logs. (This set of text is not in the commit message.)
• If migration changes causes an issue, it is much easier to see what code was changed (a simple search on MIGRATION) without having to dig through commits.

The downside is:

• The CLI is adding comments to the code!
• Too big of a change this late in the RC process?

Possibly two options then:

• Commit migration changes automatically Y/N (default to N)
• Add migration comments to changed code Y/N (default to Y)

[yjaaidi]

What about a dedicated log file in some exploitable format, yaml? Explaining which files have changed and why.
It can be quite annoying to have the same comment block on every migrated part of the code... or maybe we need a schematic to remove the comments 😅

[mhartington]

2cents here

If it's going to be done automatically, it should be squashed into one commit. But then that opens a lot of possibilities for errors and doesn't really provide a clear way to see what was changed.

If it's going to be opt-in, then it could prompt the user asking if they wanted those changes to be committed.

TL;DR, don't automatically commit changes 😄

[IgorMinar]

This was resolved by changing the default to not commit and instead provide it as an option via --create-commits.

Thanks for the feedback everyone.

[angular-automatic-lock-bot]

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.}