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 versioning to plugin add command #916

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

Conversation

theoretick
Copy link

@theoretick theoretick commented Apr 5, 2021

Summary

Allows user to specify version along with URL when adding plugin to lock
to a specific version instead of always relying on HEAD.

❯ git clone https://github.com/theoretick/asdf.git ~/.asdf --branch support-plugin-versions
Cloning into '/Users/theoretick/.asdf'...
remote: Enumerating objects: 8, done.
remote: Counting objects: 100% (8/8), done.
remote: Compressing objects: 100% (8/8), done.
remote: Total 5913 (delta 0), reused 1 (delta 0), pack-reused 5905
Receiving objects: 100% (5913/5913), 1.10 MiB | 12.68 MiB/s, done.
Resolving deltas: 100% (3400/3400), done.

❯ source ~/.asdf/asdf.sh

❯ asdf plugin add java https://github.com/halcyon/asdf-java.git

❯ asdf plugin add java https://github.com/halcyon/asdf-java.git travis-fix

❯ asdf plugin list
java
java-travis-fix

❯

Other Information

Relates to #234

@theoretick
Copy link
Author

@jthegedus would you have a chance to look over this change soon or would there be another review I could pass this one to?

@theoretick
Copy link
Author

@jthegedus sorry for the bump, would you be the best person to review this one or would there be another team member who could look over this change?

@jthegedus
Copy link
Contributor

Sorry mate. I'll be reviewing this the next few days. Trying to get on top of the existing PRs so we can move forward on new changes at greater speed.

Appreciate the work you have contributed.

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.

This is looking good so far.

I would like this change to also include what happens when asdf plugin update [<name> | --all] is called on a ref that is a tag. Currently it will ditch the tag and go back to the HEAD of the default branch.

Is that something you have time to look at @theoretick ?

@theoretick theoretick force-pushed the support-plugin-versions branch 2 times, most recently from af16cd2 to cc8ed51 Compare August 8, 2021 00:15
@theoretick
Copy link
Author

Is that something you have time to look at @theoretick ?

@jthegedus sorry for the delay but done with db90b80. I don't love the solution but please let me know what you think!

Allows user to specify version along with URL when adding plugin to lock
to a specific version instead of always relying on HEAD.

Relates to asdf-vm#234
@theoretick theoretick marked this pull request as draft August 21, 2021 14:16
@theoretick theoretick force-pushed the support-plugin-versions branch 2 times, most recently from 403c8c9 to 5f9fdc9 Compare August 21, 2021 15:20
@theoretick
Copy link
Author

This is looking good so far.

I would like this change to also include what happens when asdf plugin update [<name> | --all] is called on a ref that is a tag. Currently it will ditch the tag and go back to the HEAD of the default branch.

Is that something you have time to look at @theoretick ?

@jthegedus So I attempted this with 58de320 but it seems this approach is at odds with #800 given

@test "asdf plugin-update should pull latest default branch (refs/remotes/origin/HEAD) for plugin even if already set to specific ref" {
explicitly updates detached HEADs to the default branch 🤔. I'm not sure how best to resolve this change in behavior. Do you have any suggestions?

@aabouzaid
Copy link

This feature is super crucial to have minimal security measurements.
Otherwise any plugin could be malformed and introduce a security breach.

@Stratus3D
Copy link
Member

Stratus3D commented Aug 7, 2024

Hi @theoretick , sorry for the late reply here. I agree with @aabouzaid this feature is important for security reasons.

However, I'm in the process of rewriting asdf in Golang, and the rewrite should be done in a month or two. I don't see much point in getting this merged right now only to be replaced by the Golang implementation. Would you consider contributing these changes to the Golang implementation in a month or two?

@theoretick
Copy link
Author

@Stratus3D sure, if I have capacity then I'd be happy to port it over and excited to see how the rewrite lands. Thanks!

@aabouzaid
Copy link

Just in case anyone is waiting for this, you can start using the community project ASDF Plugin Manager right away and control asdf plugins via .plugin-versions.

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.

4 participants