-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fixing node-join script for macos to support app bundles #50188
base: master
Are you sure you want to change the base?
Conversation
lib/web/scripts/node-join/install.sh
Outdated
TELEPORT_ARCHIVE_PATH='{{.packageName}}' | ||
TELEPORT_BINARY_DIR="/usr/local/bin" | ||
TELEPORT_BINARY_LIST="teleport tctl tsh teleport-update" | ||
TELEPORT_BINARY_LIST_darwin="teleport" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not critical because there's only one item, but this should be declared as an array if it's going to be used as one below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other stuff got removed. I kept this as just a string to keep it consistent. The original TELEPORT_BINARY_LIST
is a space separated list of tools and the loops are based on that.
I don't think we need any client tools when running this script 🤔 So, I would instead remove any client binary from the installation. |
Ah that's fair. I was following current convention for most of this since it was a regression for macOS after v17 release. To keep current functionality on Linux I can keep that and just remove the client libraries for macOS since that doesn't work right now (script will error on darwin). |
Let's try to get Marco's comments addressed so we can merge this soon. |
d9c4449
to
6c04055
Compare
macOS distribution methods were updated to include Application Bundles for
tsh.app
andtctl.app
replacing the standalone binaries. Other installation methods were updated for this change but this script was forgotten. The node join script should now successfully install on macOS.Fixes #50171
changelog: Fixed a bug that prevented the node-join script from working on macOS.