-
Notifications
You must be signed in to change notification settings - Fork 16
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
Trust check & installation improvements #38
Conversation
* Also let installation install all trusted tools before bailing
bail!( | ||
"Tool {name} is not trusted. \ | ||
Run `aftman trust {name}` in your terminal to trust it.", |
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.
Note that we no longer exit the process here because that would make "greedy" installation of as many trusted tools as possible, impossible. We still bail and that will prevent any callers of trust_check
from using it incorrectly. I assume this was done for safety reasons but since there are lints for unhandled Result
types it should be hard to miss if anything needs to use trust_check
in the future.
fn trust_status(&self, name: &ToolName) -> anyhow::Result<TrustStatus> { | ||
let trusted = TrustCache::read(&self.home)?; | ||
let is_trusted = trusted.tools.contains(name); | ||
if is_trusted { | ||
Ok(TrustStatus::Trusted) | ||
} else { | ||
Ok(TrustStatus::NotTrusted) | ||
} | ||
} |
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.
This method essentially does the same thing as trust_check
did at the top before, but in the same spirit of TrustMode
having a TrustStatus
here instead of a bool
makes things more explicit and allows for easier expansion of trust behavior in the future.
@@ -135,15 +136,35 @@ impl ToolStorage { | |||
} | |||
|
|||
/// Install all tools from all reachable manifest files. | |||
pub fn install_all(&self, trust: TrustMode) -> anyhow::Result<()> { | |||
pub fn install_all(&self, trust: TrustMode, skip_untrusted: bool) -> anyhow::Result<()> { |
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.
The skip_untrusted
flag might be better handled by giving back the vec of errors to the caller of install_all
here, but it didn't really look great when I tried and would leave some room for not properly handling those errors.
``` | ||
|
||
Install all tools listed in `aftman.toml` files based on your current directory. | ||
|
||
If `--no-trust-check` is given, all tools will be installed, regardless of whether they are known. This should generally only be used in CI environments. To trust a specific tool before running `aftman install`, use `aftman trust <tool>` instead. | ||
|
||
If `--skip-untrusted` is given, only already trusted tools will be installed, others will be skipped and not emit any errors. |
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.
There might be a way to word this better? I'm not super familiar with what the use case was as the related issue doesn't contain much detail about use cases.
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.
Sorry for the long turnaround on this and thank you for addressing all the feedback on the design. I'm comfortable moving forward with this; it's a good improvement!
In preparation of #26 and continuing on from my closed PR in #30, this PR refactors the
install_all
method to split trust checks from the installation process itself. This will allow tools to be installed concurrently in the future and also makes for a slightly improved user experience.After the above work was done, adding the
--skip-untrusted
flag was trivial so I went ahead and also added that in this same PR, based on feedback comments in the previous PR.Closes #6
Closes #17