Make Quay importer more robust in the face of errors #2038
Merged
Conversation
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
Reviewer's GuideThis PR fortifies the Quay importer by gracefully handling HTTP errors for deleted repos, adding configurable HTTP/HTTPS support, reducing log noise, and introducing robust wiremock-based tests with sample fixtures. Sequence diagram for error handling when fetching deleted Quay repositoriessequenceDiagram
participant QuayWalker
participant QuayImporter
participant HTTPClient
participant Logger
participant ReportBuilder
QuayWalker->>QuayImporter: repository_url(namespace, name)
QuayWalker->>HTTPClient: GET repo URL
HTTPClient-->>QuayWalker: HTTP error (e.g. 404)
QuayWalker->>Logger: warn("Error fetching repo {url}: {err}")
QuayWalker->>ReportBuilder: add_error(Phase::Retrieval, url, err)
QuayWalker-->>QuayWalker: Continue without panicking
Entity relationship diagram for QuayImporter with new 'unencrypted' fielderDiagram
QUAYIMPORTER {
string source
string namespace
integer concurrency
boolean unencrypted
}
QUAYIMPORTER ||--o| REPOSITORY : fetches
REPOSITORY {
string namespace
string name
}
Class diagram for updated QuayImporter and QuayWalkerclassDiagram
class QuayImporter {
+String source
+Option<String> namespace
+Option<usize> concurrency
+bool unencrypted
+repositories_url(page: usize): String
+repository_url(namespace: &str, name: &str): String
-scheme(): &str
}
class QuayWalker {
+QuayImporter importer
+fetch(reference: &Reference): Option<Vec<u8>>
+run()
}
class Client {
+OciClient client
+RegistryAuth auth
+new(unencrypted: bool)
}
QuayWalker --> QuayImporter
QuayWalker --> Client
Client --> OciClient
OciClient --> RegistryAuth
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2038 +/- ##
==========================================
+ Coverage 68.01% 68.64% +0.62%
==========================================
Files 362 362
Lines 20221 20240 +19
Branches 20221 20240 +19
==========================================
+ Hits 13754 13894 +140
+ Misses 5680 5556 -124
- Partials 787 790 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jcrossley3
force-pushed
the
TC-2686
branch
2 times, most recently
from
20b6b29 to
5343e28
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `modules/importer/src/runner/quay/walker.rs:198-207` </location>
<code_context>
+ match (&repo.namespace, &repo.name) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Error handling for repository fetches is improved, but fallback to 'repo' may mask issues.
Using 'repo' as a fallback could cause downstream code to misinterpret errors as successful fetches. Recommend returning an explicit error or sentinel value instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This affirms the downstream bug, https://issues.redhat.com/browse/TC-2686 It also reduces some log noise. There is no fix in this commit, only a failing test.
Because of the way the OCI client works, we need the 'source' to be a 'registry', not a URL. We'll construct the URL given both a scheme and a registry. This will ensure we can mock our OCI client calls
Includes configuration of OCI client for unencrypted HTTP
|
Successfully created backport PR for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
In particular, when repos in the master list are deleted from quay.io before the list is fully processed.
This affirms the downstream bug, https://issues.redhat.com/browse/TC-2686
It also reduces some log noise.
Summary by Sourcery
Improve the Quay importer’s resilience to deleted or erroring repositories by catching HTTP errors, logging warnings, and continuing processing; add support for HTTP scheme; streamline tracing instrumentation; and introduce wiremock-backed tests.
New Features:
unencryptedflag to QuayImporter to control HTTP vs HTTPS for registry requestsEnhancements:
unencryptedsettingDocumentation:
unencryptedimporter option in the OpenAPI schemaTests:
Chores:
wiremockas a dependency in the root and importer Cargo.toml files#[instrument(skip(...))]attributes withskip_allto reduce log noise