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.

Sign up for GitHub

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

Jump to bottom

build: update to husky@6 #41405

Closed
wants to merge 2 commits into from
Closed

build: update to husky@6 #41405

wants to merge 2 commits into from

Conversation

damingerdai
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label Apr 1, 2021
@pullapprove pullapprove bot requested a review from gkalpak April 1, 2021 04:34
gkalpak
gkalpak approved these changes Apr 1, 2021
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thx, @damingerdai!
Should we switch to prepare npm script (instead of postinstall) as is the new recommended way (after typicode/husky#890)?

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: build & ci Related the build and CI infrastructure of the project target: patch This PR is targeted for the next patch release labels Apr 1, 2021
@ngbot ngbot bot modified the milestone: Backlog Apr 1, 2021
@gkalpak
Copy link
Member

gkalpak commented Apr 1, 2021

BTW, you need to add a commit message body (of at least 20 characters) to pass linting.

@damingerdai
Copy link
Contributor Author

Should we switch to prepare npm script (instead of postinstall) as is the new recommended way (after typicode/husky#890)?

I agree with you, but I am not sure whether I should complete the changes in this pr.

@damingerdai
Copy link
Contributor Author

@gkalpak please review

@gkalpak
Copy link
Member

gkalpak commented Apr 2, 2021

@damingerdai, I don't see any changes. Did you forget to push them maybe?

@damingerdai
Copy link
Contributor Author

@damingerdai, I don't see any changes. Did you forget to push them maybe?

i just add commit message body.

@gkalpak
Copy link
Member

gkalpak commented Apr 2, 2021

Oh, I see, sorry 😅
I thought you implemented switching to prepare and was looking for code changes 😇

I think it is reasonable to switch to prepare as part of this PR, since it is a new recommendation in husky 6, so it makes sense to make the change along with the updating imo.

@damingerdai
Copy link
Contributor Author

@gkalpak please review

gkalpak
gkalpak requested changes Apr 3, 2021
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thx for making the changes, @damingerdai 👍

Can you please change the second commit message to mention typicode/husky#890, where the recommendation to switch to prepare is made. For example:

build: install husky in `prepare` script instead of `postinstall`

With typicode/husky#890, the recommended way to install husky is in the
`prepare` script instead of the `postinstall`. This commit moves
the husky installation to the `prepare` script to align with the new
recommendation.

package.json Outdated Show resolved Hide resolved
@damingerdai
Copy link
Contributor Author

@gkalpak thanks for your suggestions. please review again.

@damingerdai damingerdai requested a review from gkalpak April 5, 2021 13:06
gkalpak
gkalpak approved these changes Apr 5, 2021
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thx again, @damingerdai 👍

LGTM, except you need to rebase on latest master to make CI happy 😃

@damingerdai
build: update to husky@6
5db9884 
Upgrade husky from 5.0.1 to 6.0.0
@damingerdai
build: install husky in prepare script instead of postinstall
f066d9c 
With typicode/husky#890, the recommended way to install husky is in the
`prepare` script instead of the `postinstall`. This commit moves
the husky installation to the `prepare` script to align with the new
recommendation.
@damingerdai
Copy link
Contributor Author

@gkalpak i have rebased on latest master but there still are two ci failed

@damingerdai damingerdai requested a review from gkalpak April 6, 2021 02:38
@damingerdai damingerdai mentioned this pull request Apr 6, 2021
Merged
gkalpak
gkalpak approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thx again, @damingerdai!
The latest CI failures were flakes. I have restarted the job and it passes now 🚀

@gkalpak gkalpak removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 6, 2021
@gkalpak gkalpak added the action: merge The PR is ready for merge by the caretaker label Apr 6, 2021
@atscott atscott closed this in a6ca790 Apr 6, 2021
atscott pushed a commit that referenced this pull request Apr 6, 2021
@damingerdai @atscott
build: install husky in prepare script instead of postinstall (#4…
acebe92 
…1405)

With typicode/husky#890, the recommended way to install husky is in the
`prepare` script instead of the `postinstall`. This commit moves
the husky installation to the `prepare` script to align with the new
recommendation.

PR Close #41405
atscott pushed a commit that referenced this pull request Apr 6, 2021
@damingerdai @atscott
build: update to husky@6 (#41405)
f39ae4b 
Upgrade husky from 5.0.1 to 6.0.0

PR Close #41405
atscott pushed a commit that referenced this pull request Apr 6, 2021
@damingerdai @atscott
build: install husky in prepare script instead of postinstall (#4…
aa36121 
…1405)

With typicode/husky#890, the recommended way to install husky is in the
`prepare` script instead of the `postinstall`. This commit moves
the husky installation to the `prepare` script to align with the new
recommendation.

PR Close #41405
@damingerdai damingerdai deleted the husky6 branch April 7, 2021 09:31
@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 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gkalpak gkalpak gkalpak approved these changes

Assignees
No one assigned
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
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@damingerdai @gkalpak