feat: Displays a warning when a plugin from the tools-version list does not exist #1033

threkk · 2021-08-29T13:02:25Z

Summary

When calling the install command, asdf tries to look for versions for all the plugins available and installs them. With this change, it will attempt to find versions for all the installed plugins and plugins defined in the .tool-versions.

Other Information

Fixes #574

jthegedus · 2021-09-03T01:21:13Z

If multiple plugins are not present on the system, will this error for the first it finds in the list, with a rerun required to then error on the next missing plugin? Assuming that is true, is there a way we could log/warn of all missing plugins as opposed to the first not found?

threkk · 2021-09-04T10:27:54Z

I have rewritten the plugin installation. This approach preserves the original installation and at the same time it will report plugin's not available. It will also not stop in case there is a missing plugin.

jthegedus · 2021-09-06T07:10:48Z

Great, thanks for the update. Can you write test cases to cover these scenarios to prevent regressions?

threkk · 2021-09-06T23:15:32Z

Sure! I will try to find time this week(end) and come back with an update :)

Stratus3D · 2021-09-07T14:46:20Z

I have rewritten the plugin installation. This approach preserves the original installation and at the same time it will report plugin's not available. It will also not stop in case there is a missing plugin.

I think we should stop installing when a missing plugin is encountered. Some projects define plugins in the order they must be installed in, because certain plugins depend on others. Proceeding to install subsequent versions after a plugin is not found could cause problems.

Stratus3D · 2021-09-07T14:53:11Z

lib/commands/command-install.bash

+  if [ -f "$tool_versions_path" ]; then
+    tools_file=$(strip_tool_version_comments "$tool_versions_path" | cut -d ' ' -f 1)
+    for plugin_name in $tools_file; do
+      if [[ ! "${plugins_installed[*]}" =~ $plugin_name ]]; then


Is this the same logic we use elsewhere in the codebase? I think this might cause problems if two plugins share matching prefixes. For example, ag and age are both existing plugins right now. With this logic, age could match the plugin name ag.

No, that is the first occurrence of that comparison. I will turn it into an exact comparison.

jthegedus · 2021-09-07T23:09:57Z

I think we should stop installing when a missing plugin is encountered.

Fine with me, but I would like the output to show all missing plugins, as opposed to just the first encountered missing so it is a showstopper only once.

threkk · 2021-09-07T23:42:46Z

The changes are then;

Fix the comparison for the plugin name (~= -. ==)
Exit if at least one plugin is missing.
Add unit tests to avoid future regresions.
Do I forget anything?

Stratus3D · 2021-09-15T13:28:22Z

The changes are then;

* Fix the comparison for the plugin name (`~=` -. `==`)

* Exit if at least one plugin is missing.

* Add unit tests to avoid future regresions.
  Do I forget anything?

I think that is all. Thanks again for your work on this. @jthegedus do you have anything to add?

jthegedus · 2021-09-15T21:54:54Z

Just for clarity (bolded my additions):

Fix the comparison for the plugin name (~= -. ==)
Compute list of all missing plugins.
Exit if at least one plugin is missing , warn with the whole list of all missing plugins.
Add unit tests to avoid future regresions.

Thanks for tackling our feedback @threkk

threkk · 2021-09-18T10:50:28Z

I made the changes discussed before. The tests run successfully in Ubuntu, but OSX tests look broken in general. Is this a known issue or do I need to change the code?

Stratus3D · 2021-09-23T16:06:22Z

I made the changes discussed before. The tests run successfully in Ubuntu, but OSX tests look broken in general. Is this a known issue or do I need to change the code?

@threkk, not sure about the tests. They are passing on the latest commit of the master branch. You could try rebasing your PR branch on top of latest master, although I'm not sure that will fix it.

Stratus3D · 2021-09-23T16:18:12Z

When I clone down your branch and run it locally (on OSX 10.15.7 (19H1323)) all but two tests pass. The two failures were:

 ✗ install_command deletes the download directory
   (in test file test/install_command.bats, line 261)
     `[ ! -d $ASDF_DIR/downloads/dummy/1.1.0 ]' failed
 ✗ asdf update --head should checkout the master branch
   (in test file test/update_command.bats, line 33)
     `[ $(git rev-parse --abbrev-ref HEAD) = "master" ]' failed
   Cloning into '/var/folders/7m/9bq62tmj60zf14d788w0lk8c0000gp/T/asdf.XXXX.4Z9epbdy/home/.asdf'...

The first test always fails for me locally (even on master) and the second only failed because I was on your branch and not the master branch when I ran the tests. So I don't see any issues here with your code. Perhaps try re-running the tests on GH or try rebasing onto latest master?

…not exist When calling the install command, it tried to look for versions for all the plugins available and installed them. With this change, it will attempt to find versions for all the installed plugins and plugins defined in the `.tool-versions`. Fixes asdf-vm#574

This patch changes the algorithm. It preserves the original logic for the plugin resolution, but at the same time, reports entries with plugins not available.

- Changes the comparison to be strict rather than partial. - Prints a list of missing plugins. - Exists if at least one plugin is not present. - Adds unit tests.

threkk · 2021-09-23T19:55:26Z

I rebased the branch and sadly the issues still persist on OSX. This seems to be some difference between OSX and Ubuntu. I will try to reproduce on a OSX machine to pin point the issue but I can't tell exactly when I will be able. If anyone has any idea about what can be wrong, I will appreciate the help :) I have my suspicions on grep behaving differently, but cannot prove it.

threkk · 2021-09-30T21:41:06Z

It took me some time, but finally found the problem: one of the flags was a *nix only flag and was causing the errors. Once removed, everything works as expected :)

jthegedus · 2021-10-01T05:46:29Z

Thanks @threkk !! We will add grep -P to our list of banned commands/flags so we don't hit this type of issue again 🙏

jthegedus

LGTM. Feel free to merge if you're happy @Stratus3D

As explained in asdf-vm#1062 asdf-vm#1033, `-P` is an option that does not exist in OSX and causes problems of compatibility.

MPV · 2021-11-04T14:56:19Z

Cool stuff! Would it be possible to cut a new release including this change?

jthegedus · 2021-11-04T21:56:12Z

We will release 0.9.0 once we finish items in the milestone and fix bugs introduced in head

cmur2 · 2021-12-11T21:07:24Z

I think we should stop installing when a missing plugin is encountered. Some projects define plugins in the order they must be installed in, because certain plugins depend on others. Proceeding to install subsequent versions after a plugin is not found could cause problems.

This change breaks my workflow as I intentionally do not install all plugins on all my machines (depending on which I need). Is there any way to disable this feature (looking at the code I guess no) or make it so it can be disabled?

Stratus3D · 2021-12-13T15:37:26Z

@cmur2 I think you make a valid point. Specifically, how do these changes break your current workflow?

We may want to provide a flag or configuration option so that missing plugins do not cause the install command to exit with a status of 1.

cmur2 · 2021-12-13T16:17:40Z

@Stratus3D I use a shared dotfiles repo between several of my machines (e.g. work and private ones) to share a .tool-versions (symlinked from the dotfiles git checkout to my home directory) among those machines so updating tools is less of a hassle (once instead of once per machine).

For work I might need kubectl which I don't use privately so it's in $HOME/.tool-versions and on the work machine I've added the asdf plugin for that but not on the private machine. This e.g. saves download bandwidth and disk storage on my private machine.

I also have other tools like nodejs which I might need at work and privately so I've added the asdf plugin on both machines.

After the update to asdf-vm v0.9.0, I cannot update nodejs on my private machine anymore since asdf complains about the missing kubectl plugin and exits with 1. It'd be cool if asdf could just ignore this state (optionally).

For now I used sed -i 's/-n "$some_plugin_not_installed"/-n ""/g' "$ASDF_DIR/lib/commands/command-install.bash" after updating asdf to workaround this.

Stratus3D · 2021-12-13T20:03:02Z

@cmur2 #1123 has been created for this. Let's continue the discussion there.

threkk requested a review from a team as a code owner August 29, 2021 13:02

threkk changed the title ~~feat: Displays a warning when the plugin from the tools-version does not exist~~ feat: Displays a warning when a plugin from the tools-version list does not exist Aug 29, 2021

Stratus3D reviewed Sep 7, 2021

View reviewed changes

threkk added 6 commits September 23, 2021 21:29

fix: linter

10734e9

ci: Removes banned commands

32e2581

feat: Changes the algorithm to detect not installed plugins

32c21eb

This patch changes the algorithm. It preserves the original logic for the plugin resolution, but at the same time, reports entries with plugins not available.

fix: Looping the wrong variable

d277bbd

fix: Addresses the pull request comments.

6ae8dbc

- Changes the comparison to be strict rather than partial. - Prints a list of missing plugins. - Exists if at least one plugin is not present. - Adds unit tests.

threkk force-pushed the master branch from 3ea4f4d to 6ae8dbc Compare September 23, 2021 19:40

fix: Removes Linux only feature

759613d

jthegedus mentioned this pull request Oct 1, 2021

bug: ban grep -P as it is *nix specific #1062

Closed

jthegedus approved these changes Oct 1, 2021

View reviewed changes

Stratus3D merged commit 9430a39 into asdf-vm:master Oct 1, 2021

threkk added a commit to threkk/asdf that referenced this pull request Oct 6, 2021

tests: Adds "grep -P" to the list of banned commands

2743a07

As explained in asdf-vm#1062 asdf-vm#1033, `-P` is an option that does not exist in OSX and causes problems of compatibility.

threkk mentioned this pull request Oct 6, 2021

fix: Adds "grep -P" to the list of banned commands #1064

Merged

threkk added a commit to threkk/asdf that referenced this pull request Oct 6, 2021

fix: Adds "grep -P" to the list of banned commands

56bdbe9

As explained in asdf-vm#1062 asdf-vm#1033, `-P` is an option that does not exist in OSX and causes problems of compatibility.

Stratus3D mentioned this pull request Dec 13, 2021

Provide a flag or configuration option to allow asdf install to continue when missing plugins are found in .tool-versions #1123

Open

tapayne88 mentioned this pull request Jan 6, 2022

Updated references to asdf version to use v0.9.0 tapayne88/dotfiles#62

Merged

quilicicf mentioned this pull request Jan 7, 2022

Feature Idea: asdf install to also install plugins #276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Displays a warning when a plugin from the tools-version list does not exist #1033

feat: Displays a warning when a plugin from the tools-version list does not exist #1033

threkk commented Aug 29, 2021 •

edited

jthegedus commented Sep 3, 2021

threkk commented Sep 4, 2021

jthegedus commented Sep 6, 2021

threkk commented Sep 6, 2021

Stratus3D commented Sep 7, 2021

Stratus3D Sep 7, 2021

threkk Sep 7, 2021

jthegedus commented Sep 7, 2021

threkk commented Sep 7, 2021

Stratus3D commented Sep 15, 2021

jthegedus commented Sep 15, 2021 •

edited

threkk commented Sep 18, 2021 •

edited

Stratus3D commented Sep 23, 2021

Stratus3D commented Sep 23, 2021

threkk commented Sep 23, 2021

threkk commented Sep 30, 2021

jthegedus commented Oct 1, 2021

jthegedus left a comment

MPV commented Nov 4, 2021

jthegedus commented Nov 4, 2021

cmur2 commented Dec 11, 2021

Stratus3D commented Dec 13, 2021

cmur2 commented Dec 13, 2021

Stratus3D commented Dec 13, 2021

feat: Displays a warning when a plugin from the tools-version list does not exist #1033

feat: Displays a warning when a plugin from the tools-version list does not exist #1033

Conversation

threkk commented Aug 29, 2021 • edited

Summary

Other Information

jthegedus commented Sep 3, 2021

threkk commented Sep 4, 2021

jthegedus commented Sep 6, 2021

threkk commented Sep 6, 2021

Stratus3D commented Sep 7, 2021

Stratus3D Sep 7, 2021

Choose a reason for hiding this comment

threkk Sep 7, 2021

Choose a reason for hiding this comment

jthegedus commented Sep 7, 2021

threkk commented Sep 7, 2021

Stratus3D commented Sep 15, 2021

jthegedus commented Sep 15, 2021 • edited

threkk commented Sep 18, 2021 • edited

Stratus3D commented Sep 23, 2021

Stratus3D commented Sep 23, 2021

threkk commented Sep 23, 2021

threkk commented Sep 30, 2021

jthegedus commented Oct 1, 2021

jthegedus left a comment

Choose a reason for hiding this comment

MPV commented Nov 4, 2021

jthegedus commented Nov 4, 2021

cmur2 commented Dec 11, 2021

Stratus3D commented Dec 13, 2021

cmur2 commented Dec 13, 2021

Stratus3D commented Dec 13, 2021

threkk commented Aug 29, 2021 •

edited

jthegedus commented Sep 15, 2021 •

edited

threkk commented Sep 18, 2021 •

edited