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

fix(ci): make husky post pull script 'sh' compatible #32269

Merged
merged 2 commits into from
Feb 14, 2024
Merged

fix(ci): make husky post pull script 'sh' compatible #32269

merged 2 commits into from
Feb 14, 2024

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Feb 14, 2024 •
edited

This is an interesting case. On most systems sh is symlinked to bash. So the existing husky scripts run without any issues on those systems.
I implemented the script on Fedora where sh -> bash. So it worked there without any issues.

Now I've destro hopped to Ubuntu based Linux Mint. Here sh doesn't redirect to bash. :(

The PR updates the shell scripts to make them compatible with sh. For testing in local try commands:

# check where 'sh' is redirected to
ls -la /bin/sh

# if not bash then run following commands to test both the scripts
rm ./.husky/_/history
sh ./.husky/post-merge

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner February 14, 2024 13:21
@OnkarRuikar OnkarRuikar requested review from bsmth and removed request for a team February 14, 2024 13:21
@github-actions github-actions bot added the system Infrastructure and configuration for the project label Feb 14, 2024
@github-actions github-actions bot added the size/s 6-50 LoC changed label Feb 14, 2024
teoli2003
teoli2003 approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

👍

Sounds good to me. Waiting some extra time before actually merging to be sure @bsmth has time to make further comments if he wants.

bsmth
bsmth approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Let's give it a spin, ty 👍🏻

@teoli2003 teoli2003 merged commit b30fd4e into mdn:main Feb 14, 2024
7 checks passed
@OnkarRuikar OnkarRuikar deleted the ci_fix_install_script branch February 14, 2024 16:45
@yin1999
Copy link
Member

yin1999 commented Feb 15, 2024 •
edited

This seems to break the CIs in translated-content: https://github.com/mdn/translated-content/actions/runs/7910611263/job/21593477827#step:6:1

Append: should we ignore git hook in CIs by adding --ignore-scripts to yarn install?

@OnkarRuikar OnkarRuikar mentioned this pull request Feb 15, 2024
Merged
@OnkarRuikar
Copy link
Contributor Author

Append: should we ignore git hook in CIs by adding --ignore-scripts to yarn install?

--ignore-scripts will disable all the scripts including preapre and postinstall. Better add a check in the update-history script itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teoli2003 teoli2003 teoli2003 approved these changes

@bsmth bsmth bsmth approved these changes

Assignees
No one assigned
Labels
size/s 6-50 LoC changed system Infrastructure and configuration for the project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@OnkarRuikar @yin1999 @teoli2003 @bsmth