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

Use #!/usr/bin/env bash in get-info.sh to support macOS users #35

Merged
merged 2 commits into from
Feb 3, 2022

Conversation

cisaacstern
Copy link
Member

get-info.sh uses the mapfile command

mapfile -t < <(kubectl logs -n "$BAKERY_NAMESPACE" deployment/prefect-agent | sed -En "s/\[([0-9]+-[0-9]+-[0-9]+) ([0-9]+:[0-9]+:[0-9]+).* agent \| Completed deployment of flow run (.*)/\1@\2-\3/p")

This does not work out-of-the-box on macOS, because even the latest Macs (mine is about ~2 months old) ship with system bash as bash 3, but mapfile was introduced in bash 4. 👈 This linked issue recommends upgrading bash with homebrew and then sudo mv /usr/local/bin/bash /bin/bash. This solution might work in a GitHub Workflows context, but on an actual MacBook, write access to /bin is blocked (even for root users) by security features which are cumbersome to circumvent.

For executing scripts with user-installed versions of bash, this tutorial recommends the #!/usr/bin/env bash syntax added in this PR. I contemplated replacing #!/bin/bash with this in all of the scripts for consistency, but thought it would be better to only make the minimal required change.

I am going to add a check for bash version to this PR, so the code self-documents why we are doing this, and then I will open for review.

xref #19 (b/c this issue arose as part of that work)

@cisaacstern
Copy link
Member Author

I am going to add a check for bash version to this PR, so the code self-documents why we are doing this, and then I will open for review.

All set! Opening for review now.

@cisaacstern cisaacstern marked this pull request as ready for review February 3, 2022 00:15
Copy link
Contributor

@tracetechnical tracetechnical left a comment

Choose a reason for hiding this comment

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

May be worth adding "which bash" to get the bash dir, incase someone has something funky going on.

@tracetechnical
Copy link
Contributor

That said... the hashbang is pretty explicit about where to find bash.. so I revoke my previous comment

@cisaacstern cisaacstern merged commit a79b2f7 into main Feb 3, 2022
@cisaacstern cisaacstern deleted the mapfile-bash-version branch February 3, 2022 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants