-
Notifications
You must be signed in to change notification settings - Fork 517
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
fix: imagefullName support more than two slash-separated #1907
Conversation
3c4c3ad
to
41e06c9
Compare
cmd/kk/pkg/images/utils.go
Outdated
@@ -173,11 +173,18 @@ func NewManifestSpec(image string, entries []manifesttypes.ManifestEntry) manife | |||
|
|||
func validateImageName(imageFullName string) error { | |||
image := strings.Split(imageFullName, "/") | |||
if len(image) != 3 { | |||
return errors.Errorf("image %s is invalid, only the format \"registry/namespace/name:tag\" is supported", imageFullName) | |||
if len(image) >= 3 { |
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.
Bigger than 3 or less than 3?
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.
only 3 or bigger than 3 is legal
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.
i get it
cmd/kk/pkg/images/utils.go
Outdated
if len(image) != 3 { | ||
return errors.Errorf("image %s is invalid, only the format \"registry/namespace/name:tag\" is supported", imageFullName) | ||
if len(image) >= 3 { | ||
return errors.Errorf("image %s is invalid,image PATH need contain at least two slash-separated", imageFullName) |
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.
Extra whitespace between contain at
is not necessary. A whitespace after comma is necessary. invalid,image
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.
ok
501b5a0
to
a7925ae
Compare
} | ||
return nil | ||
} | ||
|
||
func suffixImageName(imageFullName []string) string { |
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.
Good to have an unit test here.
/lgtm |
LGTM label has been added. Git tree hash: 7adfa3e79ac543566167f4924026047eda13eaa8
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: littleBlackHouse, wongearl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
for imagefullName support more than two slash-separated, like 10.121.218.184:30002/cache/goharbor/notary-server-photon:v2.8.2, While the OCI Distribution Specification supports more than two slash-separated components
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: