Draft
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
srid
reviewed
Aug 13, 2025
shivaraj-bh
requested changes
Aug 19, 2025
| relevant_checks.push(&health.caches); | ||
| }; | ||
| // TODO: Re-calculate NixInfo since our nix.conf has changed (due to `cachix use`) | ||
| // TODO: Re-calculate NixInfo since our nix.conf has changed (due to `cachix use`, `attic login` and `attic use`) |
Member
There was a problem hiding this comment.
attic login doesn’t change nix.conf
Comment on lines
65
to
+104
| let missing = health.caches.get_missing_caches(nix_info); | ||
| let (missing_cachix, missing_other) = parse_many(&missing, CachixCache::from_url); | ||
| let (missing_cachix, remaining_after_cachix) = | ||
| parse_many(&missing, |cache_spec| match cache_spec { | ||
| omnix_health::check::caches::CacheSpec::Cachix(name) => { | ||
| Some(CachixCache(name.clone())) | ||
| } | ||
| _ => None, | ||
| }); | ||
| let (missing_attic, missing_other) = parse_many(&remaining_after_cachix, |cache_spec| { | ||
| match cache_spec { | ||
| omnix_health::check::caches::CacheSpec::Attic { .. } => { | ||
| // Convert back to URL string and use existing parsing logic | ||
| let url_string = cache_spec.to_url_string(); | ||
| AtticCache::from_url_string(&url_string) | ||
| } | ||
| _ => None, | ||
| } | ||
| }); | ||
|
|
||
| for cachix_cache in &missing_cachix { | ||
| tracing::info!("🐦 Running `cachix use` for {}", cachix_cache.0); | ||
| cachix_cache.cachix_use().await?; | ||
| } | ||
|
|
||
| for attic_cache in &missing_attic { | ||
| tracing::info!( | ||
| "🏺 Running `attic login {}` for server {}", | ||
| attic_cache.server_name, | ||
| attic_cache.cache_url | ||
| ); | ||
| attic_cache.attic_login().await?; | ||
|
|
||
| tracing::info!( | ||
| "🏺 Running `attic use {}:{}`", | ||
| attic_cache.server_name, | ||
| attic_cache.cache_name | ||
| ); | ||
| attic_cache.attic_use().await?; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
You can instead:
let missing. = health.caches.get_missing_caches(nix_info);
for cache in &missing {
match cache {
Cachix(url_string) => { … }
Attic {server_name, cache_url } => { … }
_ => { //Do nothing }
}
}
| /// Use `om develop` for automatic installation of Cachix and Attic caches. | ||
| #[derive(Debug, Serialize, PartialEq, Eq, Clone)] | ||
| #[serde(rename_all = "kebab-case")] | ||
| pub enum CacheSpec { |
Member
There was a problem hiding this comment.
You could call it CacheType? I would’ve gone with Caches but that name is already taken
| #[serde(rename_all = "kebab-case")] | ||
| pub enum CacheSpec { | ||
| /// Regular HTTP/HTTPS cache URL (must be added manually to Nix config) | ||
| Regular(String), |
Comment on lines
+17
to
+22
| Cachix(String), | ||
| /// Attic cache (automatically installed by `om develop` via `attic login` + `attic use`) | ||
| Attic { | ||
| server_name: String, | ||
| cache_url: String, | ||
| }, |
Member
There was a problem hiding this comment.
why not Cachix(CachixCache) and Attic(AtticCache) respectively?
This would also avoid having to convert to the respective cache type after parsing missing caches during om develop above.
Comment on lines
+56
to
+65
| ### Cache Types | ||
|
|
||
| omnix supports checking different types of binary caches: | ||
|
|
||
| - **Cachix caches**: Use standard HTTPS URLs like `https://yourproject.cachix.org` | ||
| - **Attic caches**: Use the format `attic+servername+https://cache.example.com/cachename` | ||
| - **Other caches**: Standard HTTPS URLs that must be manually configured in your Nix configuration | ||
|
|
||
| **Note**: `om health` only checks if these caches are configured. For automatic cache setup, use [`om develop`](develop.md#caches) with direnv. | ||
|
|
Member
There was a problem hiding this comment.
om health doesn’t care about the cache type, it just validates if the url string is configured in nix.conf. Remove the section and just add a note callout.
| /// Run `cachix use` for this cache | ||
| /// Run `cachix use` for this cache. | ||
| /// | ||
| /// **Called by:** `om develop` only (not `om health`) |
Member
There was a problem hiding this comment.
Caller info is unnecessary here
|
|
||
| /// Run `attic use` for this cache (assumes login has already been done). | ||
| /// | ||
| /// **Called by:** `om develop` only (not `om health`) |
Member
There was a problem hiding this comment.
Caller info is unnecessary here
|
|
||
| /// Run `attic login` for this cache server. | ||
| /// | ||
| /// **Called by:** `om develop` only (not `om health`) |
Member
There was a problem hiding this comment.
Caller info is unnecessary here
| /// | ||
| /// **Called by:** `om develop` only (not `om health`) | ||
| /// **Requires:** ATTIC_LOGIN_TOKEN environment variable | ||
| pub async fn attic_login(&self) -> anyhow::Result<()> { |
Member
There was a problem hiding this comment.
Add a TODO in the doc to remove this function after zhaofengli/attic#243 is resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #474