Skip to content

Commit

Permalink
Address PR feedback
Browse files Browse the repository at this point in the history
- Rename create_image_source to image_source for terseness
- Add specific daemon_source and registry_source helper functions
- Remove daemon prefix references in code
- Remove feature list from README (use changelog instead)
- Add CI support to ensure Docker images are available for testing
- Remove unnecessary comments
- Make tests more robust
  • Loading branch information
jssblck committed Feb 28, 2025
1 parent 6ead7d3 commit 6b36abe
Show file tree
Hide file tree
Showing 7 changed files with 120 additions and 39 deletions.
9 changes: 9 additions & 0 deletions .github/workflows/check-dynamic.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ jobs:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ github.token }}

# Pull a small test image to ensure we have at least one image in the Docker daemon for tests
- name: Pull alpine test image (Linux/Mac)
if: ${{ matrix.settings.host == 'ubuntu-latest' || matrix.settings.host == 'macos-latest' }}
run: docker pull alpine:latest

- name: Pull alpine test image (Windows)
if: ${{ matrix.settings.host == 'windows-latest' }}
run: docker pull alpine:latest

- run: cargo nextest run --all-targets
- run: cargo test --doc
Expand Down
4 changes: 0 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -227,10 +227,6 @@ by setting `RUST_LOG=circe=debug` or `RUST_LOG=circe_lib=debug`.
> In macOS and Linux, you can apply environment variables to a command without changing your environment;
> for example: `RUST_LOG=trace circe ...`.
### features and improvements

- [x] Automatic Docker daemon check: Circe now automatically checks the local Docker daemon first before pulling from remote registries, which can significantly speed up operations when images are already available locally.

#### future improvements

These are somewhat "known issues", but mostly "things to keep in mind" when using `circe`.
Expand Down
4 changes: 2 additions & 2 deletions bin/src/extract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ pub async fn main(opts: Options) -> Result<()> {

let output = canonicalize_output_dir(&opts.output_dir, opts.overwrite)?;

// Use the factory function to create the appropriate image source
let source = circe_lib::create_image_source(
// Get the appropriate image source
let source = circe_lib::image_source(
reference,
Some(auth),
opts.target.platform,
Expand Down
4 changes: 2 additions & 2 deletions bin/src/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ pub async fn main(opts: Options) -> Result<()> {
_ => Authentication::docker(&reference).await?,
};

// Use the factory function to create the appropriate image source
// Get the appropriate image source
let source =
circe_lib::create_image_source(reference, Some(auth), opts.target.platform, None, None)
circe_lib::image_source(reference, Some(auth), opts.target.platform, None, None)
.await
.context("create image source")?;

Expand Down
1 change: 0 additions & 1 deletion lib/src/daemon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ impl Daemon {
.context("list images")?;

for image in images {
// RepoTags is a Vec, not an Option
if image.repo_tags.iter().any(|tag| tag == &image_name) {
return Ok(true);
}
Expand Down
89 changes: 59 additions & 30 deletions lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,49 @@ pub trait ImageSource: Send + Sync {
) -> Result<(), color_eyre::Report>;
}

/// Creates an appropriate image source, trying Docker daemon first and then fallback to registry.
pub async fn create_image_source(
/// Creates a Docker daemon image source.
pub async fn daemon_source(
reference: Reference,
platform: Option<Platform>,
layer_filters: Option<Filters>,
file_filters: Option<Filters>,
) -> Result<daemon::Daemon> {
let daemon = daemon::Daemon::builder()
.reference(reference)
.maybe_platform(platform)
.layer_filters(layer_filters.unwrap_or_default())
.file_filters(file_filters.unwrap_or_default())
.build()
.await?;

Ok(daemon)
}

/// Creates a registry image source.
pub async fn registry_source(
reference: Reference,
auth: Option<Authentication>,
platform: Option<Platform>,
layer_filters: Option<Filters>,
file_filters: Option<Filters>,
) -> Result<registry::Registry> {
let registry = registry::Registry::builder()
.maybe_platform(platform)
.reference(reference)
.auth(auth.unwrap_or(Authentication::None))
.layer_filters(layer_filters.unwrap_or_default())
.file_filters(file_filters.unwrap_or_default())
.build()
.await?;

Ok(registry)
}

/// Returns an appropriate image source, trying Docker daemon first and then falling back to registry.
///
/// This is a helper function that attempts to use a local Docker daemon image if available,
/// and falls back to pulling from the registry if not.
pub async fn image_source(
reference: Reference,
auth: Option<Authentication>,
platform: Option<Platform>,
Expand All @@ -47,23 +88,15 @@ pub async fn create_image_source(
) -> Result<Box<dyn ImageSource>> {
// Check if Docker daemon is available
if daemon::is_daemon_available().await {
// Create a corresponding daemon reference
let daemon_reference = Reference::builder()
.host("daemon".to_string())
.repository(reference.repository.clone())
.version(reference.version.clone())
.build();

let daemon = daemon::Daemon::builder()
.reference(daemon_reference.clone())
.maybe_platform(platform.clone())
.layer_filters(layer_filters.clone().unwrap_or_default())
.file_filters(file_filters.clone().unwrap_or_default())
.build()
.await;
let daemon_result = daemon_source(
reference.clone(),
platform.clone(),
layer_filters.clone(),
file_filters.clone(),
).await;

// Only use daemon if it's available and the image exists
if let Ok(daemon) = daemon {
if let Ok(daemon) = daemon_result {
if let Ok(true) = daemon.image_exists().await {
info!("Image found in Docker daemon, using local copy");
return Ok(Box::new(daemon));
Expand All @@ -72,18 +105,14 @@ pub async fn create_image_source(
}
}

// If we get here, either:
// 1. The daemon isn't available
// 2. The image doesn't exist in the daemon
// So use the registry
let registry = registry::Registry::builder()
.maybe_platform(platform)
.reference(reference.clone())
.auth(auth.unwrap_or(Authentication::None))
.layer_filters(layer_filters.unwrap_or_default())
.file_filters(file_filters.unwrap_or_default())
.build()
.await?;
// Fall back to registry
let registry = registry_source(
reference,
auth,
platform,
layer_filters,
file_filters,
).await?;

Ok(Box::new(registry))
}
Expand Down Expand Up @@ -535,7 +564,7 @@ impl FromStr for Reference {
}
}

// No special case for daemon prefix - we'll automatically check the daemon anyway
// Parse the reference components

// Docker supports `docker pull ubuntu` and `docker pull library/ubuntu`,
// both of which are parsed as `docker.io/library/ubuntu`.
Expand Down
48 changes: 48 additions & 0 deletions lib/tests/it/daemon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,51 @@ async fn list_daemon_images() -> Result<()> {

Ok(())
}

// This test verifies that the image_source function correctly selects the Docker daemon
// when the image is available locally
#[test_case("docker.io/library/alpine:latest"; "alpine")]
#[test_log::test(tokio::test)]
async fn auto_daemon_selection(image: &str) -> Result<()> {
// Skip the test if Docker daemon is not available
if !circe_lib::daemon::is_daemon_available().await {
eprintln!("skipping test; Docker daemon not available");
return Ok(());
}

// First ensure we have the image in the daemon
let reference = image.parse::<Reference>()?;
let daemon = Daemon::builder().reference(reference.clone()).build().await?;

// If image doesn't exist in daemon, pull it first
if !daemon.image_exists().await? {
eprintln!("Image not in daemon, pulling it...");
let tmp = TempDir::new().await?;
let registry = circe_lib::registry::Registry::builder()
.reference(reference.clone())
.build()
.await?;

let layers = registry.layers().await?;
for layer in layers {
let path = tmp.dir_path().join(layer.digest.as_hex());
registry.apply_layer(&layer, &path).await?;
}

// Verify the image is now in the daemon
assert!(daemon.image_exists().await?, "Image should now be in the daemon");
}

// Now test the image_source function - it should select the daemon
let source = circe_lib::image_source(reference, None, None, None, None).await?;

// Get layers and ensure we can read them - verifying the source works
let layers = source.layers().await?;
assert!(!layers.is_empty(), "Image should have at least one layer");

// Ensure we get a registry source when passing a non-existent image
let nonexistent_ref = "docker.io/library/nonexistent:latest".parse::<Reference>()?;
let source = circe_lib::image_source(nonexistent_ref, None, None, None, None).await?;

Ok(())
}

0 comments on commit 6b36abe

Please sign in to comment.