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.

Sign up for GitHub

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

Jump to bottom

feat: Add versioning to plugin add command #916

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

feat: Add versioning to plugin add command #916

wants to merge 3 commits into from

Conversation

theoretick
Copy link

@theoretick theoretick commented Apr 5, 2021 •
edited
Loading

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 theoretick requested a review from a team as a code owner April 5, 2021 16:28
@theoretick theoretick mentioned this pull request Apr 5, 2021
Closed
@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 jthegedus mentioned this pull request May 19, 2021
Closed
@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.

jthegedus
jthegedus requested changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@jthegedus jthegedus left a comment •
edited
Loading

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!

@theoretick
feat: Add versioning to plugin add command
a593511 
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

asdf/test/plugin_update_command.bats

Line 59 in 95f2cdf

@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.

@aabouzaid aabouzaid mentioned this pull request Feb 14, 2023
Merged
@Stratus3D
Copy link
Member

Stratus3D commented Aug 7, 2024 •
edited
Loading

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
Reviewers

@jthegedus jthegedus Awaiting requested review from jthegedus

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@theoretick @jthegedus @aabouzaid @Stratus3D