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

feat: add plugin location when update the plugin #1602

Merged
merged 5 commits into from
Sep 10, 2023

Conversation

edvardsanta
Copy link
Contributor

@edvardsanta edvardsanta commented Jul 21, 2023

It is a feature created by recommendation of issue 1573.

Summary

Display plugin location when calling plugin update

❯ asdf plugin-update ruby
Plugin ruby location: /home/user/.asdf/plugins/ruby
Updating ruby to master
Already on 'master'
Your branch is up to date with 'origin/master'.

Closes: #1573

Currently that is just

❯ asdf plugin-update ruby
Updating ruby to master
Already on 'master'
Your branch is up to date with 'origin/master'.

It is a feature created by recommendation of issue 1573.
@edvardsanta edvardsanta requested a review from a team as a code owner July 21, 2023 00:39
@edvardsanta
Copy link
Contributor Author

Observation:

In the test code, i tried to implement get_full_path to check the output with expected output , the get_full_path function in the test appears to be returning only the plugin name instead of the full path to the plugin. I don´t know if it is the expected behavior

@hyperupcall
Copy link
Contributor

I don't see any get_full_path function, but get_plugin_path seems to work for me:

@test "asdf plugin-update prints the location of plugin (specific)" {
  local plugin_path
  plugin_path="$(get_plugin_path dummy)"

  run asdf plugin-update dummy

  local expected_output="Plugin dummy location: $plugin_path"
  [[ "$output" == *"$expected_output"* ]]
}
...
 ✓ asdf plugin-update prints the location of plugin (specific)

@hyperupcall
Copy link
Contributor

hyperupcall commented Jul 29, 2023

LGTM, my only comment is that the wording might be a little confusing - perhaps "Location of 'ruby' plugin: " would fit better?

Edit: I have added this PR to the list of ones that I would like to see merged for the upcoming v0.13.0 release.

@hyperupcall
Copy link
Contributor

Unfortunately, I cannot run the GitHub actions test workflow, but I have ran locally and it seems there a few other failing tests such as asdf plugin-update executes configured pre hook (generic).

It looks like current tests don't take into account that this can be printed first. It looks like adding an asterisk to the glob test fixes things:

[[ "$output" = *"UPDATE dummy"*"${expected_output}" ]]

@edvardsanta
Copy link
Contributor Author

Thanks for your suggestions, i will resee my code and check the tests this week

@edvardsanta
Copy link
Contributor Author

I did the changes, thanks for your help. I am trying to figure out why the other tests are failing, I tried removing my piece of code, but it's still not working

image

@hyperupcall
Copy link
Contributor

hyperupcall commented Aug 15, 2023

Hmm, I only get the same error as before:

✗ asdf plugin-update executes configured pre hook (specific)
   (in test file test/plugin_update_command.bats, line 188)
     `[[ "$output" = "UPDATE"*"${expected_output}" ]]' failed

It might be an issue with your computer only . The --print-output-on-failure flag may help diagnose the issue.

@edvardsanta
Copy link
Contributor Author

Hi, I made the changes according to what you said
image

@edvardsanta
Copy link
Contributor Author

Hello, it was a simple change, is there any impediment or problem that I can solve for this pull request to be finished?

@hyperupcall
Copy link
Contributor

@edvardsanta I'm not sure if you forgot to push your changes, I'm still getting the error:

$ bats test/plugin_update_command.bats --print-output-on-failure
plugin_update_command.bats
 ✓ asdf plugin-update should pull latest default branch (refs/remotes/origin/HEAD) for plugin
 ✓ asdf plugin-update should pull latest default branch (refs/remotes/origin/HEAD) for plugin even if default branch changes
 ✓ asdf plugin-update should pull latest default branch (refs/remotes/origin/HEAD) for plugin even if the default branch contains a forward slash
 ✓ asdf plugin-update should pull latest default branch (refs/remotes/origin/HEAD) for plugin even if already set to specific ref
 ✓ asdf plugin-update should not remove plugin versions
 ✓ asdf plugin-update should not remove plugins
 ✓ asdf plugin-update should not remove shims
 ✓ asdf plugin-update done for all plugins
 ✓ asdf plugin-update executes post-plugin update script
 ✓ asdf plugin-update executes post-plugin update script if git-ref updated
 ✓ asdf plugin-update executes configured pre hook (generic)
 ✗ asdf plugin-update executes configured pre hook (specific)
   (in test file test/plugin_update_command.bats, line 188)
     `[[ "$output" = "UPDATE"*"${expected_output}" ]]' failed
   Last output:
   Location of dummy plugin: /tmp/asdf with spaces.9nU1/home/.asdf/plugins/dummy
   UPDATE
   Updating dummy to master
   Already on 'master'
   Your branch is up to date with 'origin/master'.
   plugin updated path=/tmp/asdf with spaces.9nU1/home/.asdf/plugins/dummy old git-ref=b809aa8 new git-ref=b809aa8
 ✓ asdf plugin-update executes configured post hook (generic)
 ✓ asdf plugin-update executes configured post hook (specific)
 ✓ asdf plugin-update prints the location of plugin (specific)

15 tests, 1 failure

@hyperupcall
Copy link
Contributor

hyperupcall commented Aug 31, 2023

Once the test is fixed, then it is up to one of the active maintainers @Stratus3D or @jthegedus to run the workflow and merge it. I'm not sure when that will be - as you can see there are currently some other PRs in the backlog.

@edvardsanta
Copy link
Contributor Author

edvardsanta commented Aug 31, 2023

this test is failing even without my feat, but i will try to figure out what is happening, thx
edit: i understand now, sorry, i must learn better bats =]
image

asdf plugin-update executes configured pre hook (specific) was failing
@edvardsanta
Copy link
Contributor Author

could u test it again?

@hyperupcall
Copy link
Contributor

hyperupcall commented Sep 7, 2023

Checked the diff and I believe the test suite will pass fully. I can't merge your PR here, but I'll cherry-pick and squash your commits (and some from other existing PRs) into my fork and make a release later today.

Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

Thanks! And thanks for the review @hyperupcall

@jthegedus jthegedus merged commit 36c7024 into asdf-vm:master Sep 10, 2023
6 of 7 checks passed
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.

Show plugin update path
3 participants