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

fix: [M3-7274] - Update curl-bash with system default /bin/bash #11021

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Oct 1, 2024

Changes 🔄

  • Removed installation scripts to encourage users to install tools and other dependencies following the step-by-step instructions.
  • /bin/bash -c is more explicit and ensures we're using the system's default bash located at /bin/bash. It aligns with HomeBrews recommended method on installation. https://brew.sh/

Note

I did not think we needed hash verification for this, but we can if we wanted to make this more of a manual step for users and increase the size of this script by adding a shasum verification step. Open to opinions.

Target release date 🗓️

N/A

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@jaalah-akamai jaalah-akamai added Ready for Review Documentation Improving / adding to our documentation labels Oct 1, 2024
@jaalah-akamai jaalah-akamai self-assigned this Oct 1, 2024
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner October 1, 2024 02:07
@jaalah-akamai jaalah-akamai requested review from bnussman-akamai and cpathipa and removed request for a team October 1, 2024 02:07
@jaalah-akamai jaalah-akamai changed the title chore: [M3-7274] - Update curl-bash with system default /bin/bash fix: [M3-7274] - Update curl-bash with system default /bin/bash Oct 1, 2024
Copy link

github-actions bot commented Oct 1, 2024

Coverage Report:
Base Coverage: 87.12%
Current Coverage: 87.12%

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Does this change actually address the issue raised by M3-7274?

With all the contributions we've been getting from outside of our team, I'm wondering if it might make sense to get rid of getting_started.sh altogether and encourage users to install tools and other dependencies following the step-by-step instructions:

  • It really doesn't take very much effort to install the needed tools manually
  • getting_started.sh was written with Mac users in mind, so we'll have to support manual installation regardless if we ever have contributors on Linux
  • It's pretty heavy handed and opinionated in terms of what it installs -- that makes sense for our team where we have consensus, but if I were a random contributor and ran this script and it unexpectedly installed Homebrew, Volta, GitHub CLI, etc., I'd be annoyed. To pick on Volta as an example, it's not even required for development and just happens to be our blessed version manager -- it would be just as valid for a contributor to install Node 20 directly or use a version manager that they already have installed

@bnussman-akamai
Copy link
Member

bnussman-akamai commented Oct 1, 2024

That's a good point @jdamore-linode . I don't know if this would fix the unsafe piping that this describes

Screenshot 2024-10-01 at 10 20 22 AM

I'm cool with removing getting_started.sh for the reasons you listed. I see value in having an install script, but I agree it is opinionated, rigid, and built for MacOS.

@jaalah-akamai jaalah-akamai merged commit a5df16d into linode:develop Oct 2, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improving / adding to our documentation Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants