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

feat(ci): add post pull script to run install #32062

Merged
merged 6 commits into from
Feb 14, 2024

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Feb 1, 2024

People have faced issues after pulling new code from origin or base and they have spent hours figuring out what was wrong. Almost all the time simple yarn install makes thing right again.

The PR adds post-merge script which runs when a contributor runs git pull or git pull upstream main. The script has following properties:

  • it works only if the branch is main
  • do yarn install on very first run
  • when pulled code has package.json modified then run yarn install <-- this is the main feature
  • do nothing if package.json hasn't been modified since last pull

Yari does yarn install on every precommit, which will be very annoying in content. Next best thing is to do it immediately after pull/merge in main branch.

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner February 1, 2024 15:56
@OnkarRuikar OnkarRuikar requested review from pepelsbey and removed request for a team February 1, 2024 15:56
@github-actions github-actions bot added system [PR only] Infrastructure and configuration for the project size/s [PR only] 6-50 LoC changed labels Feb 1, 2024
@OnkarRuikar
Copy link
Contributor Author

ping @dipikabh @teoli2003

@dipikabh
Copy link
Contributor

dipikabh commented Feb 2, 2024

Thanks for adding this feature, @OnkarRuikar. It indeed can be frustrating to realize that all that was needed was a yarn install after trying everything else.

If package.json has changed, what do you think about giving a warning message after pull/merge in main branch, suggesting to run yarn install, rather than doing it automatically?

.husky/post-merge Outdated Show resolved Hide resolved
@OnkarRuikar
Copy link
Contributor Author

I've updated the feature. Now it gives a warning message instead of actually running the install command.
Also as suggested by @estelle the warning will be shown on every pull till you run the install command. Then till the package.json get's modified there will be no warning on pulls.

Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

Thanks for adding the warning message, @OnkarRuikar

.husky/post-merge Outdated Show resolved Hide resolved
.husky/update-history Outdated Show resolved Hide resolved
.husky/update-history Outdated Show resolved Hide resolved
.husky/update-history Outdated Show resolved Hide resolved
@dipikabh
Copy link
Contributor

Hi @OnkarRuikar, feel free to ping one of us to merge this after you've had a chance to fix the failing check. Thanks!

@sideshowbarker sideshowbarker merged commit b3063b0 into mdn:main Feb 14, 2024
14 of 15 checks passed
@OnkarRuikar OnkarRuikar deleted the ci_run_install branch February 14, 2024 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/s [PR only] 6-50 LoC changed system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants