-
Notifications
You must be signed in to change notification settings - Fork 787
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
feat: install script for logging on installation #739
Conversation
git checkout "$(git describe --abbrev=0 --tags)" | ||
curl -o- https://raw.githubusercontent.com/asdf-vm/asdf/v0.7.8/install.bash | bash | ||
# or | ||
wget -qO- https://raw.githubusercontent.com/asdf-vm/asdf/v0.7.8/install.bash | bash |
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.
I don't think we should adopt this pattern. I know it's popular but it has some risks and I don't think we should push our users in this direction.
See:
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.
I agree with all of this. Given it is the install method of choice for Homebrew & OMZSH users I didn't think it was too much an ask, especially since others can still just git clone
. Additionally, my understanding of prior discussion of package manager code in this repo was that we didn't want to maintain those either.
But, we don't necessarily need to use this method for people who are using the docs site - see #739 (comment)
printf "%s\t\t\t%s\n" "1: install asdf" "completed!" | ||
printf "%s\t%s\n" "2: add asdf to your shell" "https://asdf-vm.com/#/core-manage-asdf-vm" | ||
printf "%s\t\t\t%s\n" "3: add a plugin" "https://asdf-vm.com/#/core-manage-plugins" | ||
printf "%s\t%s\n" "4: install a tool version" "https://asdf-vm.com/#/core-manage-versions" |
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.
I'm not sure that providing links provides much benefit to the user. Why can't we just tell the user what do to add asdf to their shell? If this script is invoked by the package manager we should be able to tell the user exactly what do to for their specific OS/package manager combination.
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.
Why can't we just tell the user what do to add asdf to their shell
I think that solution is unmaintainable for us. The number of permutations is making the docs difficult to update as is. nvm
does this and their script is 100s of lines of conditional shell scripting which it almost certainly doesn't work in all cases. I'd rather maintain the docs and get users to them instead of automatically editing peoples shell configs.
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.
I'm not sure that providing links provides much benefit to the user
The idea was to capture users who hear about asdf
and just run their package manager of choices installation, eg brew install asdf
, and haven't ever come to the documentation site. These users have find that it doesn't work, they were given no further instructions, and instead of looking for a documentation site to see if there is something they missed, they check GitHub for issues where people used the same package manager and report they themselves are having issues.
I would like to show these people 2 things:
- there's more steps before you can use
asdf
- these steps are on our documentation site
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.
Given we don't need to inform users who are installing from the documentation site about the existence of the site, we could leave the docs as is, and only use the install script in Homebrew and co after they clone the repo as they currently do.
I will create an Issue in the repo to pin for Homebrew users to direct them to the documentation. As most Google the issue and end up finding old Issues and resolving the problem via an old method. Edit: added with #785 |
Summary
Use an install script to perform the
git clone
installation process. This allows us to log on installation which is required for the Homebrew installation as outlined in #738 (Homebrew users expectbrew install asdf
to perform the complete setup of asdf and subsequently report a GitHub issue before going to the docs site to see further instructions)Fixes: #738
Other Information
sed
expert)curl
/wget
the install script from the GitHub remote--git-dir
and--work-tree
to avoid filesystem traversal asgit
provides this functionalityNotes
git checkout
uses--quiet
flag to supress "detached head" warning on tag checkout. Perhaps we let it log this?ASDF_DATA_DIR
instead of introducing another env varTesting output:
with
--quiet
:without
--quiet
: