-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add fallback input option to ensure that fallback is not used #517
Conversation
@@ -22,3 +26,4 @@ runs: | |||
env: | |||
INPUT_TOOL: ${{ inputs.tool }} | |||
INPUT_CHECKSUM: ${{ inputs.checksum }} | |||
INPUT_FALLBACK: ${{ inputs.fallback }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest INPUT_STRATEGIES
and pass it directly to cargo-binstall
.
User can specify crate-meta-data
for trying to download it from official maintainer, quick-install
from third-party quickinstall, compile
for from source.
If empty, then disable cargo-binstall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting approval for cargo-binstall would be worth attempting IMO if its cargo-quickinstall was disabled.
i.e. trying to explain to my cyber-security team that we're using install-action which falls back to cargo-binstall which falls back to cargo-quickinstall is too hard, and it is that cargo-quickinstall stage which becomes "oh its just grabbing some binary from somewhere on the internet, but they are good people running it, trust them". The fall back to cargo-binstall is explainable if it is "it fetches from github releases or it builds from source 100%"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good to hear, disabling quickinstall is pretty easy, you just need to pass --strategies crate-meta-data,compile
and it will only use the prebuilt from the official repository specified in Cargo.toml
on https://crates.io , or fallback to cargo-install
.
--strategies compile
would then be equivalent to launching cargo-install
, though cargo-binstall
would launch multiple cargo-install
for each crate to compile them in parallel, with a jobserver to limit parallelism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, for a reviewer who is not familiar with the details of cargo-binstall's API but needs to review code that uses this action, something like strategies: ''
/strategies: crate-meta-data,compile
is not at all clear at first glance. (compared to something like fallback: none
/fallback: binstall-first-party
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, maybe we can pass --disable-strategies
instead.
--disable-strategies quick-install
is equivalent to --strategies crate-meta-data,compile
, and it's much more straight forward for people just wanting to disable quickinstall.
d5ee81d
to
4a88678
Compare
Considering there is a proposal to add a non-cargo-binstall fallback (#526) too, I think this option is good as is. We can discuss how to pass additional flags to cargo-binstall in another issue. |
Add an option to prevent accidentally installing tools that use fallback for installation.
We could add an option to use cargo-binstall without quickinstall in the future if someone actually wants to use that.
Related discussion: #514
cc @jayvdb