Add import commands for repos and publishes#43
Add import commands for repos and publishes#43andrewshadura wants to merge 3 commits intocollabora:mainfrom
Conversation
YAML is useful as it’s slightly more compact and readable than JSON. This requires the serde_yaml dependency. Fixes: collabora#40 Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
394e249 to
98ba3f2
Compare
Add import functionality to create repositories and publishes from JSON or YAML configuration files or stdin: # Import from file aptlyctl repo import repos.json aptlyctl publish import publishes.yaml # Import from stdin cat repos.json | aptlyctl repo import Conflicts are handled as follows: - Before doing anything, we verify that none of the repos/publishes we are about to create exist; if any do, we abort. - If --skip-existing is in use, we allow repos/publishes to exist; we don't touch them though. - We never delete or recreate anything, as this poses a risk of data loss. Both JSON and YAML are supported as input formats. If format is unspecified, stdin is always parsed as JSON. For files, format is selected by the extension. Fixes: collabora#39 Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
98ba3f2 to
35b2677
Compare
There was a problem hiding this comment.
Pull request overview
Adds import support to aptlyctl so repositories and published repositories can be created from JSON/YAML configuration (file or stdin), and extends existing list-style commands to support YAML output.
Changes:
- Add
repo importandpublish importcommands that read JSON/YAML from a file or stdin and create resources (optionally skipping existing). - Add
Yamlto the globalOutputFormatand implement YAML output for repo/publish/snapshot listing (and repo package listing). - Extend
aptly-restpublish models to includeskip_contents, and addserde_yamldependency.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| aptlyctl/src/main.rs | Adds OutputFormat::Yaml to support YAML output across commands. |
| aptlyctl/src/snapshot.rs | Adds YAML output option for snapshot listing. |
| aptlyctl/src/repo.rs | Adds YAML output and introduces repo import for creating repos from JSON/YAML. |
| aptlyctl/src/publish.rs | Adds YAML output and introduces publish import for creating published repos from JSON/YAML. |
| aptlyctl/Cargo.toml | Adds serde_yaml dependency to enable YAML parsing/printing. |
| aptly-rest/src/api/publish.rs | Adds skip_contents field + accessor on PublishedRepo for import parity. |
| Cargo.lock | Locks new YAML-related dependencies (serde_yaml, unsafe-libyaml). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let existing = aptly.published().await?; | ||
|
|
||
| let conflicts: Vec<_> = publishes | ||
| .iter() | ||
| .filter(|p| { | ||
| existing.iter().any(|e| { | ||
| e.prefix() == p.prefix() && e.distribution() == p.distribution() | ||
| }) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
The conflict detection and per-item skip check scan existing with iter().any(...) for each imported publish, giving O(n*m) behavior. Since you already build a HashSet for repo import, consider building a HashSet<(prefix, distribution)> here as well to make conflict checks and skipping O(1) and simplify the code.
| for repo in publishes { | ||
| if existing.iter().any(|e| { | ||
| e.prefix() == repo.prefix() && e.distribution() == repo.distribution() | ||
| }) { | ||
| info!( | ||
| "Published repository '{}/{}' already exists, skipping", | ||
| repo.prefix(), | ||
| repo.distribution() | ||
| ); |
There was a problem hiding this comment.
Similar to repo import: duplicates within the input (same prefix + distribution appearing more than once) aren't detected up front, and the existing list isn't updated after a successful creation. This can cause a mid-import failure and leave a partially imported state. Consider pre-validating uniqueness of (prefix, distribution) within publishes and/or tracking newly created entries in a set as you go.
| let repos: Vec<repos::Repo> = match args.format { | ||
| Some(OutputFormat::Json) => serde_json::from_slice(&content)?, | ||
| Some(OutputFormat::Yaml) => serde_yaml::from_slice(&content)?, | ||
| Some(OutputFormat::Name) => { | ||
| return Err(color_eyre::eyre::eyre!( | ||
| "Name format is not supported for import" | ||
| )); | ||
| } |
There was a problem hiding this comment.
RepoImportOpts.format reuses OutputFormat, so --format name will be accepted by clap but then fails at runtime with "Name format is not supported for import". Consider using a dedicated value enum for import formats (json/yaml only) or otherwise preventing name from being a valid CLI value here to avoid a surprising UX.
| for repo in repos { | ||
| if existing.contains(repo.name()) { | ||
| info!("Repo '{}' already exists, skipping", repo.name()); | ||
| skipped += 1; | ||
| } else { | ||
| aptly.create_repo(&repo).await?; | ||
| info!("Created repo '{}'", repo.name()); |
There was a problem hiding this comment.
Import checks existing only once (pre-import) and doesn't validate duplicates within the input itself. If the input contains the same repo name twice, the second create will fail mid-import, leaving a partially imported state. Consider detecting duplicate names in the parsed input up front (and erroring), and/or updating the in-memory existing set after each successful creation.
| Some(OutputFormat::Name) => { | ||
| return Err(color_eyre::eyre::eyre!( | ||
| "Name format is not supported for import" | ||
| )); | ||
| } | ||
| None => { | ||
| // Infer from file extension |
There was a problem hiding this comment.
PublishImportOpts.format reuses OutputFormat, so --format name will be accepted by clap but then fails at runtime with "Name format is not supported for import". Consider using a dedicated value enum for import formats (json/yaml only) or otherwise preventing name from being a valid CLI value here to avoid a surprising UX.
| Some(OutputFormat::Name) => { | |
| return Err(color_eyre::eyre::eyre!( | |
| "Name format is not supported for import" | |
| )); | |
| } | |
| None => { | |
| // Infer from file extension | |
| _ => { | |
| // Infer from file extension (or default to JSON for stdin) |
Add import functionality to create repositories and publishes from JSON or YAML configuration files or stdin:
Conflicts are handled as follows:
Both JSON and YAML are supported as input formats.
If format is unspecified, stdin is always parsed as JSON.
For files, format is selected by the extension.
Some settings (e.g.
--skip-bz2) cannot be picked up as they’re either not exported by the APIlistuses or not accepted by the object creation API. As far as I know, newer aptly versions improve this situation a bit.This PR fixes #39.