-
Notifications
You must be signed in to change notification settings - Fork 112
Backports/1.4/dockerhub workaround #3495
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
Backports/1.4/dockerhub workaround #3495
Conversation
Reviewer's GuideThis backport introduces a DockerHub API workaround in fetch_remote_tags to accommodate tags with no image entries and supplements the suite with an integration test for TagFetcher in the versionchooser service. Sequence diagram for DockerHub tag processing workaround in fetch_remote_tagssequenceDiagram
participant TagFetcher
participant DockerHubAPI
participant TagMetadata
TagFetcher->>DockerHubAPI: Request tags for repository
DockerHubAPI-->>TagFetcher: Return tags (some with images, some without)
loop For each tag
TagFetcher->>TagMetadata: If tag["images"] is empty, create TagMetadata with digest="------"
TagFetcher->>TagMetadata: Else, create TagMetadata for each image with matching architecture
end
Class diagram for updated TagMetadata handling in fetch_remote_tagsclassDiagram
class TagMetadata {
+repository: str
+image: str
+tag: str
+last_modified: str
+sha: str | None
+digest: str
}
class TagFetcher {
+fetch_remote_tags(repository: str, local_images: List[str])
}
TagFetcher --> TagMetadata : creates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- The new TestTagFetcher integration test depends on DockerHub and may be flaky—consider marking it as an integration test (e.g. with a custom pytest marker) or adding a timeout/mocking to improve reliability.
- In fetch_remote_tags, avoid using the magic string "------" for the digest placeholder—extract it into a named constant or use None to make the fallback more explicit.
- The test’s broad except Exception block can hide real failures; catch only network-related errors or specific exceptions when skipping due to connectivity issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new TestTagFetcher integration test depends on DockerHub and may be flaky—consider marking it as an integration test (e.g. with a custom pytest marker) or adding a timeout/mocking to improve reliability.
- In fetch_remote_tags, avoid using the magic string "------" for the digest placeholder—extract it into a named constant or use None to make the fallback more explicit.
- The test’s broad except Exception block can hide real failures; catch only network-related errors or specific exceptions when skipping due to connectivity issues.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Seems like a reasonably straightforward cherry-pick backport of the specified functionality 👍
) | ||
valid_images.append(tag) | ||
continue | ||
for image in tag["images"]: |
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.
for image in tag["images"]: | |
for image in images: |
If they've already been fetched into a variable, it makes sense to use that variable, right?
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.
Since this is a cherry-pick from master, it would be nice to move this change to master not here
This is a backport of #3490 into 1.4
Summary by Sourcery
Backport DockerHub workaround into 1.4 by synthesizing TagMetadata for tags without image entries and adding an integration test to ensure TagFetcher fetches and validates remote tags correctly.
New Features:
Bug Fixes:
Enhancements:
Tests: