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

Fixing node-join script for macos to support app bundles #50188

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

doggydogworld
Copy link
Contributor

@doggydogworld doggydogworld commented Dec 13, 2024

macOS distribution methods were updated to include Application Bundles for tsh.app and tctl.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.

lib/web/scripts/node-join/install.sh Outdated Show resolved Hide resolved
lib/web/scripts/node-join/install.sh Outdated Show resolved Hide resolved
lib/web/scripts/node-join/install.sh Outdated Show resolved Hide resolved
lib/web/scripts/node-join/install.sh Outdated Show resolved Hide resolved
TELEPORT_ARCHIVE_PATH='{{.packageName}}'
TELEPORT_BINARY_DIR="/usr/local/bin"
TELEPORT_BINARY_LIST="teleport tctl tsh teleport-update"
TELEPORT_BINARY_LIST_darwin="teleport"
Copy link
Contributor

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.

Copy link
Contributor Author

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.

lib/web/scripts/node-join/install.sh Outdated Show resolved Hide resolved
@marcoandredinis
Copy link
Contributor

I don't think we need any client tools when running this script 🤔
Installing client tools has their own docs.
AFAIK, this script is only meant to create a teleport service with ssh_service.enabled=true and have it join the cluster.

So, I would instead remove any client binary from the installation.

@doggydogworld
Copy link
Contributor Author

I don't think we need any client tools when running this script 🤔 Installing client tools has their own docs. AFAIK, this script is only meant to create a teleport service with ssh_service.enabled=true and have it join the cluster.

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).

@zmb3
Copy link
Collaborator

zmb3 commented Jan 1, 2025

Let's try to get Marco's comments addressed so we can merge this soon.

@doggydogworld doggydogworld force-pushed the gus/fix-node-join-macos-script branch from d9c4449 to 6c04055 Compare January 14, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error on installation on macOS
4 participants