-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
2ba0c67
to
0b8cb55
Compare
0b8cb55
to
9d631d7
Compare
758427e
to
dbad895
Compare
dbad895
to
1a328c5
Compare
60e5416
to
59432c8
Compare
The linter seems to be unhappy:
|
59432c8
to
912bea9
Compare
@radu-matei wrote:
Oops. Fixed. Apologies. |
912bea9
to
44b874b
Compare
` | ||
|
||
type relocateCmd struct { | ||
// args |
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.
We don't usually have comments to separate args, flags and other things here, but don't really have anything against them -- and might actually start using them in other places as well.
@@ -53,3 +53,7 @@ | |||
[[constraint]] | |||
name = "github.com/docker/go" | |||
version = "1.5.1-1" | |||
|
|||
[[constraint]] |
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.
Looking at https://github.com/pivotal/image-relocation -- would it make sense to have an initial release we can pinpoint here?
Not necessarily blocking this PR though.
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.
Possibly, although that would make most sense if go-containerregistry was also released, which it hasn't been so far. Let's defer for now.
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.
@glyn - For helm3 the push/pull utilizes https://github.com/deislabs/oras which is based on containerd @bacongobbler do you think there could be a release of oras for CNAB if needed?
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.
Thanks. The plan is to merge this PR with its implementation in terms of go-containerregistry. The implementation can be changed in the future as appropriate.
Passing an inexistent registry prefix errors out in a rather non-intuitive way:
|
Hello Glynn, as we discussed in the issue, I think we should try to improve cnab-to-oci Fixup instead. If I understand it correctly, the only blocking point you saw in A second argument for not merging this PR as-is, is that it adds a dependency on |
@silvin-lubecki Let's take the discussion over to the issue as I think it's more appropriate there. |
cnabio/cnab-go#4 needs merging and we need to upgrade master of this repo to include that before we can resolve the |
I'll address this in the image-relocation repo: vmware-archive/image-relocation#6 |
A few weeks back we brought this up in the CNAB meeting, and it was decided that, at least for now, we want to have both the My opinion is that we'd rather have this piece of functionality merged, and work to converging the two approaches after, than waiting for it -- I'm really interested in the approaches you raised, and am working on the push/pull functionality, so let's continue the discussion about using But in the meantime, I think we should have this merged -- thoughts? |
I'm totally fine with this 👍. Thanks to @glyn we have a clear functional scope, and if it is merged now we can improve |
Ok, happy we're all back on the same page here. @glyn -- once the conflicts are fixed, I think this first iteration is good to go. |
f0d1b64
to
8e8c3c2
Compare
This is now fixed. The error is now:
|
cmd/duffle/relocate.go
Outdated
repositoryValue := cmd.Flag(flagName).Value.String() | ||
|
||
if strings.HasSuffix(repositoryValue, "/") || strings.Contains(repositoryValue, "//") { | ||
return fmt.Errorf("invalid repository: %s", repositoryValue) |
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.
As you have here a clear error case, maybe you can improve the error message and tell what is the exact issue in the repository name?
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.
Fixed.
cmd/duffle/relocate.go
Outdated
for i, part := range strings.Split(repositoryValue, "/") { | ||
if i != 0 { | ||
if strings.ContainsAny(part, ":@\" ") { | ||
return fmt.Errorf("invalid repository: %s", repositoryValue) |
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.
Same here?
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.
Fixed.
cmd/duffle/relocate.go
Outdated
PreRunE: func(cmd *cobra.Command, args []string) error { | ||
// validate --repository-prefix if it is set, otherwise allow flag omission to be diagnosed as such | ||
if cmd.Flags().Changed("repository-prefix") { | ||
if err := flagValidRepository("repository-prefix")(cmd); err != nil { |
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.
Does this function really need to take this string argument and return a functor? As it is only used once, it can be simplified. You can also always check the repository prefix value set:
PreRunE: func(cmd *cobra.Command, args []string) error {
if err := validateRepository(relocate. repoPrefix); err != nil{
return err
}
with bellow, replacing func flagValidRepository(flagName string) flagsValidator {
:
func validateRepository(repository string) error {
if strings.HasSuffix(repository, "/") || strings.Contains(repository, "//") {
return fmt.Errorf(...)
...
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 got rid of the functor as suggested. (Functors made more sense in the project where this code originated since there was a lot of flag validation going on.)
Note that PreRunE
deliberately falls through when the repository prefix is not specified so that we get the cobra error message for a missing required flag (for consistency). I've changed the comment to make this clearer.
8e8c3c2
to
3da57f1
Compare
@silvin-lubecki Thanks for your comments. Changes force pushed. |
See `duffle relocate -h` for documentation. Adds the originalImage field to images in both the image map and invocation images. Fixes cnabio#668
3da57f1
to
2a33de2
Compare
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.
Thanks @glyn, the overall code looks good to me 👍
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.
LGTM.
Thanks a lot for your patience here, @glyn -- this looks great for a first iteration.
See
duffle relocate -h
for documentation.Adds the
originalImage
andoriginalDigest
fields to images in both the image map and invocation images on the assumption that cnabio/cnab-spec#157 will be implemented.Deferred:
Fixes #668