return error when no tag or latest tag is specified
#4814
Conversation
Reviewer's GuideThis PR enhances bundle URI validation by rejecting empty or ‘latest’ tags with specific errors and adds corresponding unit tests to cover these scenarios. Class diagram for updated GetBundleNameFromURI functionclassDiagram
class BundleMetadata {
+GetBundleNameFromURI(bundleURI string) (string, error)
}
class Error {
<<interface>>
}
BundleMetadata ..> Error
note for BundleMetadata "Now returns specific errors for missing or 'latest' tag in docker bundle URIs"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant BundleMetadata
User->>CLI: Provide Docker bundle URI
CLI->>BundleMetadata: Call GetBundleNameFromURI(uri)
alt URI has empty tag or tag is "latest"
BundleMetadata-->>CLI: Return error
CLI-->>User: Show error message
else URI is valid
BundleMetadata-->>CLI: Return bundle name
CLI-->>User: Proceed with bundle setup
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @redbeam - I've reviewed your changes - here's some feedback:
- Consider defining and returning distinct error variables (e.g. ErrNoTag, ErrUnsupportedLatest) instead of fmt.Errorf so callers and tests can match on error values rather than error text.
- To make the tag extraction more robust, use strings.LastIndex to split on the final colon instead of strings.Split, which can misbehave if there are extra colons in the string.
- Standardize the error message formatting (capitalization and punctuation) between the "no tag" and "latest" cases to keep messaging consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider defining and returning distinct error variables (e.g. ErrNoTag, ErrUnsupportedLatest) instead of fmt.Errorf so callers and tests can match on error values rather than error text.
- To make the tag extraction more robust, use strings.LastIndex to split on the final colon instead of strings.Split, which can misbehave if there are extra colons in the string.
- Standardize the error message formatting (capitalization and punctuation) between the "no tag" and "latest" cases to keep messaging consistent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // URI with colon but no tag | ||
| bundleName, err = GetBundleNameFromURI("docker://quay.io/crcont/openshift-bundle:") | ||
| assert.Equal(t, "", bundleName) | ||
| assert.Error(t, err) |
There was a problem hiding this comment.
better to match the error string which we are expecting instead just asserting it as error.
There was a problem hiding this comment.
Or follow the recommendation from CodeRabbit even if this is not what we do in the rest of the codebase:
Consider defining and returning distinct error variables (e.g. ErrNoTag, ErrUnsupportedLatest) instead of fmt.Errorf so callers and tests can match on error values rather than error text.
There was a problem hiding this comment.
I added two error types (NoTagError and UnsupportedTagError) that are returned. Alternatively, they could be merged into a generic TagError type.
Issue #4520 We don't use 'latest' tag in container image repositories, so a check was added to return an error if the user tries to use it. Additionally, added a check for empty tag as a result of leaving a ':' at the end of the URI.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@redbeam: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
github-project-automation
bot
moved this from Ready for review
to Done
in Project planning: crc
Fixes: #4520
Type of change
test, version modification, documentation, etc.)
Testing
Contribution Checklist
Summary by Sourcery
Require explicit version tags in bundle URIs by rejecting empty or 'latest' tags and updating tests accordingly.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests