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

fix: plugin test git-ref default to the default branch of the plugin's repository #1694

Merged

Conversation

javiergarea
Copy link
Contributor

Summary

It implements the strategy followed by #800 to retrieve the plugin's default branch instead of relying on master.

Fixes: #974

Copy link
Contributor

@hyperupcall hyperupcall left a comment

Choose a reason for hiding this comment

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

I'm not a maintainer, only a frequent contributor - thanks for the patch - I noticed an error with it:

This code changes the behavior in cases where plugin_name and plugin_url are empty. Instead of a nice error message, a weird git error will show instead. Since this code executes git and is now a point of failure (ex. if plugin_url is empty), this code should idealy be placed at least after this error check or right before plugin_gitref is used.

Another thing to note, is that there is another place in the code with similar functionality, and this sort of thing could be extracted into it's own function. That said, I'm not sure if that is necessary right now, and this kind of thing can be tracked in #1697 anyways.

lib/commands/command-plugin-test.bash Outdated Show resolved Hide resolved
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 all for raising and reviewing 🙏

@jthegedus jthegedus merged commit 6d8cf9d into asdf-vm:master Jan 9, 2024
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.

Need to specify branch name for plugin testing
3 participants