Repository navigation
Add other url formats for certificate inspect - #653
Conversation
github-actions
Bot
added
the
needs triage
There was a problem hiding this comment.
Have you tested the behavior with unpassed port number? Does it reliably error put? If not, we should error check this one, or set the defaults of those to 443.
There was a problem hiding this comment.
By default unpassed port are set to 443
cli/command/certificate/remote.go
Line 39 in 5c1724a
I just set the default to 443 in the map, it make more sense.
There was a problem hiding this comment.
Also add some relevant documentations about the additions, including in the ussage part.
8f96da6 to
76ee643
Compare
c7eeb29 to
569e87a
Compare
areed
left a comment
areed
left a comment
There was a problem hiding this comment.
Test cases in remote_test.go also need to be updated
| } | ||
| if _, _, err := net.SplitHostPort(u.Host); err != nil { | ||
| port := uint64(urlPrefixes[prefix]) | ||
| u.Host += ":" + strconv.FormatUint(port, 10) |
There was a problem hiding this comment.
Could you use net.JoinHostPort here to handle IPv6
| func trimURL(ref string) (string, bool, error) { | ||
| tmp := strings.ToLower(ref) | ||
| for _, prefix := range urlPrefixes { | ||
| for prefix, _ := range urlPrefixes { |
There was a problem hiding this comment.
| for prefix, _ := range urlPrefixes { | |
| for prefix := range urlPrefixes { |
This is what the linter is looking for
LecrisUT
left a comment
LecrisUT
left a comment
There was a problem hiding this comment.
Also more documentation is found in command/certificate/inspect
| if err != nil { | ||
| return "", false, errors.Wrapf(err, "error parsing URL '%s'", ref) | ||
| } | ||
| if _, _, err := net.SplitHostPort(u.Host); err != nil { |
There was a problem hiding this comment.
For the sake of consistency, please use and return the err object, even if it would logically be handled by the previous one.
There was a problem hiding this comment.
I'm not sure I understand.
I think this error should not be returned, it is the same code as :
cli/command/certificate/remote.go
Lines 45 to 47 in 26bd65c
Please clarify what you want 😃
There was a problem hiding this comment.
I mean do a similar err return as the lines above
There was a problem hiding this comment.
This looks correct as is since the err is used to detect the case where there is no port provided.
Add other url formats with default port has requested in #459