deps: Upgrade open-component-model/ocm to v0.4.0 #1808
Conversation
kyma-bot
added
the
size/XL
lindnerby
changed the title
kyma-bot
added
size/XXL
|
|
||
| cmd.CurrentStep.Success() | ||
| if cmd.opts.RegistryURL == "" { | ||
| return nil |
There was a problem hiding this comment.
return nil is not expected, right?
There was a problem hiding this comment.
It is, the flow just got inverted since this is the last step it can be skipped if there is no reg given
There was a problem hiding this comment.
but if no regstry given, this create module command is meaningless, right? It can't push image to remote.
There was a problem hiding this comment.
i guess it's like this because the command could also be used for just persisting the archive locally to inspect layers? also the registry flag is also not mandatory which is weird if the command should be meaningless without it. i don't know the intentions/requirements when this command was implemented, but agree that this should have a second look at some point.
and it's not only the registry flow that seems inconsistent: also the generation of the template could be interesting to be used without registry for debugging purposes, but at the moment the flow ends there
pkg/module/resources.go
Outdated
| @@ -111,10 +90,10 @@ func generateResources(log *zap.SugaredLogger, version string, credLabel []byte, | |||
| r.Input.Type = "file" | |||
| } | |||
|
|
|||
| if len(credLabel) != 0 { | |||
| if len(credMatchLabels) != 0 { | |||
There was a problem hiding this comment.
I don't know why, but it looks strange to me, I'd prefer the following:
| if len(credMatchLabels) != 0 { | |
| if len(credMatchLabels) > 0 { |
| @@ -100,9 +100,7 @@ func (r *Remote) getCredentials(ctx cpi.Context) credentials.Credentials { | |||
| return creds | |||
| } | |||
|
|
|||
| // See: github.com/open-component-model/ocm/pkg/contexts/credentials/repositories/dockerconfig/repository.go#IsEmptyAuthConfig() | |||
There was a problem hiding this comment.
Maybe leave this comment? It indicates that the function is written as the one linked.
There was a problem hiding this comment.
The url results in not found, that's why I removed it.
| } | ||
|
|
||
| // addSources adds the sources to the component descriptor. If the def.Source is a git repository | ||
| func addSources(cd *ocm.ComponentDescriptor, def *Definition, gitRemote string) error { | ||
| if strings.HasSuffix(def.Source, ".git") { |
There was a problem hiding this comment.
This condition was added as part of #1751. Is it still working?
There was a problem hiding this comment.
The overall flow of this adding of git source meta information is very illogical and needed some changes. One of these is I made this function explicit, see addGitSources
pkg/module/build.go
Outdated
| nil, | ||
| nil, | ||
| vfs.ModePerm, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
This check has no effect
| if err := addSources(cd, def, gitRemote); err != nil { | ||
| return nil, err | ||
| } | ||
| file, err := archiveFs.Create("component-descriptor.yaml") |
There was a problem hiding this comment.
Maybe I am wrong, but it looks like it's bypassing the ocm functionality - in the previous version this file was created by the library, isn't it?
There was a problem hiding this comment.
Yes because of the newly introduced immutability the name and version on the descriptor must be passed before and the only way to achieve that we could find was by creating this intermediate descriptor
pkg/module/build.go
Outdated
| } | ||
|
|
||
| return archive, nil | ||
| defer file.Close() |
There was a problem hiding this comment.
I would move this even before file.Write in line 49.
| if err != nil { | ||
| return err | ||
| } | ||
| descriptor.SetLabels([]ocmv1.Label{{ |
There was a problem hiding this comment.
Why is this removed?
There was a problem hiding this comment.
Labels are added here:
Line 69 in 4bbba59
Description
Changes proposed in this pull request:
Related issue(s)