From f72a23060fa4ea261dece0ca901a6922e77861fe Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Tue, 14 Nov 2023 16:48:48 +0100 Subject: [PATCH 1/9] Fix logging --- internal/cli/logs.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/cli/logs.go b/internal/cli/logs.go index adf4050c0..f9a003ed7 100644 --- a/internal/cli/logs.go +++ b/internal/cli/logs.go @@ -9,6 +9,7 @@ import ( "go.uber.org/zap" "go.uber.org/zap/zapcore" "k8s.io/klog/v2" + ctrllog "sigs.k8s.io/controller-runtime/pkg/log" ) // NewLogger returns the logger used for CLI log output (used in Hydroform deployments) @@ -25,6 +26,7 @@ func NewLogger(printLogs bool) *zap.Logger { logger = zap.NewNop() } logr := zapr.NewLoggerWithOptions(logger) + ctrllog.SetLogger(zapr.NewLoggerWithOptions(zap.NewNop())) klog.SetLogger(logr) ocm.DefaultContext().LoggingContext().SetBaseLogger(logr) ocm.DefaultContext().LoggingContext().SetDefaultLevel(9) From 0759fc3eeacff57ab7232420178457c20c79c5c5 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Fri, 24 Nov 2023 14:19:29 +0100 Subject: [PATCH 2/9] Implementation for the difference detection --- cmd/kyma/alpha/create/module/module.go | 25 +++++--- pkg/module/remote.go | 83 +++++++++++++++++++++----- 2 files changed, 84 insertions(+), 24 deletions(-) diff --git a/cmd/kyma/alpha/create/module/module.go b/cmd/kyma/alpha/create/module/module.go index 69849b556..b7a7cd9b0 100644 --- a/cmd/kyma/alpha/create/module/module.go +++ b/cmd/kyma/alpha/create/module/module.go @@ -128,7 +128,8 @@ Build a Kubebuilder module my-domain/modC in version 3.2.1 and push it to a loca "Uses the host filesystem instead of in-memory archiving to build the module.", ) - cmd.Flags().BoolVar(&o.ArchiveVersionOverwrite, "module-archive-version-overwrite", false, "Overwrites existing component's versions of the module. If set to false, the push is a No-Op.") + cmd.Flags().BoolVar(&o.ArchiveVersionOverwrite, "module-archive-version-overwrite", false, + "Overwrites existing component's versions of the module. If set to false, the push is a No-Op.") cmd.Flags().StringVar( &o.GitRemote, "git-remote", "origin", @@ -184,7 +185,8 @@ Build a Kubebuilder module my-domain/modC in version 3.2.1 and push it to a loca &o.PrivateKeyPath, "key", "", "Specifies the path where a private key is used for signing.", ) - cmd.Flags().BoolVar(&o.KubebuilderProject, "kubebuilder-project", false, "Specifies provided module is a Kubebuilder Project.") + cmd.Flags().BoolVar(&o.KubebuilderProject, "kubebuilder-project", false, + "Specifies provided module is a Kubebuilder Project.") configureLegacyFlags(cmd, o) @@ -213,7 +215,8 @@ func configureLegacyFlags(cmd *cobra.Command, o *Options) *cobra.Command { cmd.Flags().StringVar(&o.Channel, "channel", "regular", "Channel to use for the module template.") - cmd.Flags().StringVar(&o.Namespace, "namespace", kcpSystemNamespace, "Specifies the namespace where the ModuleTemplate is deployed.") + cmd.Flags().StringVar(&o.Namespace, "namespace", kcpSystemNamespace, + "Specifies the namespace where the ModuleTemplate is deployed.") return cmd } @@ -356,12 +359,17 @@ func (cmd *command) Run(ctx context.Context) error { cmd.CurrentStep.Failure() return err } - componentVersionAccess, err := remote.Push(archive, cmd.opts.ArchiveVersionOverwrite) + componentVersionAccess, isPushed, err := remote.Push(archive, cmd.opts.ArchiveVersionOverwrite) if err != nil { cmd.CurrentStep.Failure() return err } - cmd.CurrentStep.Successf("Module successfully pushed") + if isPushed { + cmd.CurrentStep.Successf("Module successfully pushed") + } else { + cmd.CurrentStep.Successf(fmt.Sprintf("Module already exists. Retrieved image from %q", + cmd.opts.RegistryURL)) + } if cmd.opts.PrivateKeyPath != "" { cmd.NewStep("Fetching and signing component descriptor...") @@ -447,7 +455,8 @@ func (cmd *command) getModuleTemplateAnnotations(modCnf *Config, crValidator val return annotations } -func (cmd *command) validateDefaultCR(ctx context.Context, modDef *module.Definition, l *zap.SugaredLogger) (validator, error) { +func (cmd *command) validateDefaultCR(ctx context.Context, modDef *module.Definition, l *zap.SugaredLogger) (validator, + error) { cmd.NewStep("Validating Default CR") var crValidator validator @@ -509,7 +518,7 @@ func (cmd *command) moduleDefinitionFromOptions() (*module.Definition, *Config, np := nice.Nice{} np.PrintImportant("WARNING: The Kubebuilder support is DEPRECATED. Use the simple mode by providing the \"--module-config-file\" flag instead.") - //legacy approach, flag-based + // legacy approach, flag-based def = &module.Definition{ Name: cmd.opts.Name, Version: cmd.opts.Version, @@ -523,7 +532,7 @@ func (cmd *command) moduleDefinitionFromOptions() (*module.Definition, *Config, return def, cnf, nil } - //new approach, config-file based + // new approach, config-file based moduleConfig, err := ParseConfig(cmd.opts.ModuleConfigFile) if err != nil { return nil, nil, err diff --git a/pkg/module/remote.go b/pkg/module/remote.go index d90b36b30..105c541fb 100644 --- a/pkg/module/remote.go +++ b/pkg/module/remote.go @@ -12,6 +12,7 @@ import ( "github.com/open-component-model/ocm/pkg/contexts/credentials/repositories/dockerconfig" oci "github.com/open-component-model/ocm/pkg/contexts/oci/repositories/ocireg" "github.com/open-component-model/ocm/pkg/contexts/ocm" + "github.com/open-component-model/ocm/pkg/contexts/ocm/accessmethods/localblob" "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" "github.com/open-component-model/ocm/pkg/contexts/ocm/cpi" "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" @@ -26,8 +27,9 @@ import ( type NameMapping ocireg.ComponentNameMapping const ( - URLPathNameMapping = NameMapping(ocireg.OCIRegistryURLPathMapping) - DigestNameMapping = NameMapping(ocireg.OCIRegistryDigestMapping) + URLPathNameMapping = NameMapping(ocireg.OCIRegistryURLPathMapping) + DigestNameMapping = NameMapping(ocireg.OCIRegistryDigestMapping) + accessLocalReferenceFieldName = "localReference" ) // Remote represents remote OCI registry and the means to access it @@ -126,45 +128,94 @@ func NoSchemeURL(url string) string { return regex.ReplaceAllString(url, "") } -// Push picks up the archive described in the config and pushes it to the provided registry. +// Push picks up the archive described in the config and pushes it to the provided registry if not existing. // The credentials and token are optional parameters -func (r *Remote) Push(archive *comparch.ComponentArchive, overwrite bool) (ocm.ComponentVersionAccess, error) { +func (r *Remote) Push(archive *comparch.ComponentArchive, overwrite bool) (ocm.ComponentVersionAccess, bool, error) { repo, err := r.GetRepository(archive.GetContext()) if err != nil { - return nil, err + return nil, false, err } if !overwrite { - versionAlreadyExists, _ := repo.ExistsComponentVersion( - archive.ComponentVersionAccess.GetName(), archive.ComponentVersionAccess.GetVersion(), - ) + versionExists, _ := repo.ExistsComponentVersion(archive.ComponentVersionAccess.GetName(), + archive.ComponentVersionAccess.GetVersion()) + + if versionExists { + versionAccess, err := repo.LookupComponentVersion( + archive.ComponentVersionAccess.GetName(), archive.ComponentVersionAccess.GetVersion(), + ) + if err != nil { + return nil, false, fmt.Errorf("could not lookup component version: %w", err) + } - if versionAlreadyExists { - return nil, fmt.Errorf("version %s already exists, please use --module-archive-version-overwrite "+ - "flag to overwrite it", archive.ComponentVersionAccess.GetVersion()) + if descriptorResourcesAreEquivalent(archive.GetDescriptor().Resources, + versionAccess.GetDescriptor().Resources) { + return versionAccess, false, nil + } else { + return nil, false, fmt.Errorf("version %s already exists with different content, please use "+ + "--module-archive-version-overwrite flag to overwrite it", + archive.ComponentVersionAccess.GetVersion()) + } } } transferHandler, err := standard.New(standard.Overwrite(overwrite)) if err != nil { - return nil, fmt.Errorf("could not setup archive transfer: %w", err) + return nil, false, fmt.Errorf("could not setup archive transfer: %w", err) } if err = transfer.TransferVersion( - common.NewLoggingPrinter(archive.GetContext().Logger()), nil, archive.ComponentVersionAccess, repo, &customTransferHandler{transferHandler}, + common.NewLoggingPrinter(archive.GetContext().Logger()), nil, archive.ComponentVersionAccess, repo, + &customTransferHandler{transferHandler}, ); err != nil { - return nil, fmt.Errorf("could not finish component transfer: %w", err) + return nil, false, fmt.Errorf("could not finish component transfer: %w", err) } - return repo.LookupComponentVersion( + componentVersion, err := repo.LookupComponentVersion( archive.ComponentVersionAccess.GetName(), archive.ComponentVersionAccess.GetVersion(), ) + + return componentVersion, err == nil, err } type customTransferHandler struct { transferhandler.TransferHandler } -func (h *customTransferHandler) TransferVersion(repo ocm.Repository, src ocm.ComponentVersionAccess, meta *compdesc.ComponentReference, tgt ocm.Repository) (ocm.ComponentVersionAccess, transferhandler.TransferHandler, error) { +func (h *customTransferHandler) TransferVersion(repo ocm.Repository, src ocm.ComponentVersionAccess, + meta *compdesc.ComponentReference, tgt ocm.Repository) (ocm.ComponentVersionAccess, transferhandler.TransferHandler, + error) { return h.TransferHandler.TransferVersion(repo, src, meta, tgt) } + +func descriptorResourcesAreEquivalent(localResources, remoteResources compdesc.Resources) bool { + if len(localResources) != len(remoteResources) { + return false + } + + localResourcesMap := map[string]compdesc.Resource{} + for _, res := range localResources { + localResourcesMap[res.Name] = res + } + + for _, res := range remoteResources { + localResource := localResourcesMap[res.Name] + if res.Name == RawManifestLayerName { + remoteAccessObject := res.Access.(*runtime.UnstructuredVersionedTypedObject).Object + _, ok := localResourcesMap[res.Name] + if !ok { + return false + } + localAccessObject := localResource.Access.(*localblob.AccessSpec) + + // Trimming 7 characters because locally the sha256 is followed by '.' but remote it is followed by ':' + if remoteAccessObject[accessLocalReferenceFieldName].(string)[7:] != localAccessObject.LocalReference[7:] { + return false + } + } else if !res.IsEquivalent(&localResource) { + return false + } + } + + return true +} From b71c05a1302440e5e9b30461275e1c98dcfd6d29 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Fri, 24 Nov 2023 15:51:54 +0100 Subject: [PATCH 3/9] Adjust E2E test --- .github/workflows/test-e2e-create-module.yml | 15 +++++++++++++ .../kyma_create_module_same_version_test.go | 22 ++++++++++++++----- tests/e2e/util.go | 10 +++++---- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/.github/workflows/test-e2e-create-module.yml b/.github/workflows/test-e2e-create-module.yml index 48769d658..e62b2a7e6 100644 --- a/.github/workflows/test-e2e-create-module.yml +++ b/.github/workflows/test-e2e-create-module.yml @@ -75,8 +75,23 @@ jobs: --insecure \ --module-config-file ./module-config.yaml \ --version $MODULE_TEMPLATE_VERSION -v \ + --sec-scanners-config ./template-operator/sec-scanners-config.yaml \ --output /tmp/module-config-template.yaml echo "MODULE_TEMPLATE_PATH=/tmp/module-config-template.yaml" >> "$GITHUB_ENV" + - name: Create a different security scanners config file for different layers + if: ${{matrix.e2e-test == 'test-same-version-module-creation'}} + run: | + cd ./template-operator + echo 'module-name: template-operator + rc-tag: 0.5.0 + dev-branch: main + protecode: + - europe-west3-docker.pkg.dev/sap-kyma-jellyfish-dev/template-operator/component-descriptors/kyma-project.io/template-operator:v1.0.0-e2e-warning + whitesource: + language: golang-mod + exclude: + - "**/test/**" + - "**/*_test.go"' > sec-scanners-config-changed.yaml - name: Verify module template if: ${{ matrix.e2e-test == 'test-moduleconfig-module-creation' || matrix.e2e-test == 'test-kubebuilder-module-creation'}} run: | diff --git a/tests/e2e/create_module/kyma_create_module_same_version_test.go b/tests/e2e/create_module/kyma_create_module_same_version_test.go index 3a5cb1cbd..78fd4f470 100644 --- a/tests/e2e/create_module/kyma_create_module_same_version_test.go +++ b/tests/e2e/create_module/kyma_create_module_same_version_test.go @@ -3,23 +3,33 @@ package create_module_test import ( "testing" - "github.com/kyma-project/cli/tests/e2e" "github.com/stretchr/testify/assert" + + "github.com/kyma-project/cli/tests/e2e" ) func Test_SameVersion_ModuleCreation(t *testing.T) { path := "../../../template-operator" registry := "http://k3d-oci.localhost:5001" configFilePath := "../../../template-operator/module-config.yaml" + secScannerConfigFile := "../../../template-operator/sec-scanners-config.yaml" + changedsecScannerConfigFile := "../../../template-operator/sec-scanners-config-changed.yaml" version := "v1.0.0" t.Run("Create same version module with module-archive-version-overwrite flag", func(t *testing.T) { - err := e2e.CreateModuleCommand(true, path, registry, configFilePath, version) + err := e2e.CreateModuleCommand(true, path, registry, configFilePath, version, secScannerConfigFile) assert.Nil(t, err) }) - t.Run("Create same version module without module-archive-version-overwrite flag", func(t *testing.T) { - err := e2e.CreateModuleCommand(false, path, registry, configFilePath, version) - assert.Equal(t, e2e.ErrCreateModuleFailedWithSameVersion, err) - }) + t.Run("Create same version module and same content without module-archive-version-overwrite flag", + func(t *testing.T) { + err := e2e.CreateModuleCommand(false, path, registry, configFilePath, version, secScannerConfigFile) + assert.Nil(t, err) + }) + + t.Run("Create same version module but different content without module-archive-version-overwrite flag", + func(t *testing.T) { + err := e2e.CreateModuleCommand(false, path, registry, configFilePath, version, changedsecScannerConfigFile) + assert.Equal(t, e2e.ErrCreateModuleFailedWithSameVersion, err) + }) } diff --git a/tests/e2e/util.go b/tests/e2e/util.go index 875f08ed4..093ed1dab 100644 --- a/tests/e2e/util.go +++ b/tests/e2e/util.go @@ -210,21 +210,23 @@ func Flatten(labels v1.Labels) map[string]string { return labelsMap } -func CreateModuleCommand(versionOverwrite bool, path, registry, configFilePath, version string) error { +func CreateModuleCommand(versionOverwrite bool, + path, registry, configFilePath, version, secScannerConfig string) error { var createModuleCmd *exec.Cmd if versionOverwrite { createModuleCmd = exec.Command("kyma", "alpha", "create", "module", "--path", path, "--registry", registry, "--insecure", "--module-config-file", configFilePath, - "--version", version, "--module-archive-version-overwrite") + "--version", version, "--module-archive-version-overwrite", "--sec-scanner-config", secScannerConfig) } else { createModuleCmd = exec.Command("kyma", "alpha", "create", "module", "--path", path, "--registry", registry, "--insecure", "--module-config-file", configFilePath, - "--version", version) + "--version", version, "--sec-scanner-config", secScannerConfig) } createOut, err := createModuleCmd.CombinedOutput() if err != nil { - if strings.Contains(string(createOut), fmt.Sprintf("version %s already exists", version)) { + if strings.Contains(string(createOut), + fmt.Sprintf("version %s already exists with different content", version)) { return ErrCreateModuleFailedWithSameVersion } return fmt.Errorf("create module command failed with err %s", err) From 44a61483f2feaaa8573e4f83e6763bbd2e4ed720 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Fri, 24 Nov 2023 16:34:14 +0100 Subject: [PATCH 4/9] Fix E2E test --- .github/workflows/test-e2e-create-module.yml | 15 ++++++----- internal/cli/logs.go | 2 -- pkg/module/remote.go | 26 +++++++++++++------ .../kyma_create_module_same_version_test.go | 16 ++++++++---- .../create_module/kyma_create_module_test.go | 8 +++++- tests/e2e/util.go | 4 +-- 6 files changed, 47 insertions(+), 24 deletions(-) diff --git a/.github/workflows/test-e2e-create-module.yml b/.github/workflows/test-e2e-create-module.yml index bb43cb277..ca49f0778 100644 --- a/.github/workflows/test-e2e-create-module.yml +++ b/.github/workflows/test-e2e-create-module.yml @@ -53,15 +53,16 @@ jobs: - name: Run create module with kubebuilder-project if: ${{ matrix.e2e-test == 'test-kubebuilder-module-creation' }} run: | + cd ./template-operator kyma alpha create module \ --name kyma-project.io/module/template-operator \ - --path ./template-operator \ + --path . \ --registry http://k3d-oci.localhost:5001 \ --insecure \ --kubebuilder-project \ --version $MODULE_TEMPLATE_VERSION -v \ --output /tmp/kubebuilder-template.yaml \ - --sec-scanners-config ./template-operator/sec-scanners-config.yaml + --sec-scanners-config sec-scanners-config.yaml echo "MODULE_TEMPLATE_PATH=/tmp/kubebuilder-template.yaml" >> "$GITHUB_ENV" - name: Run create module with module-config if: ${{ matrix.e2e-test == 'test-moduleconfig-module-creation' || matrix.e2e-test == 'test-same-version-module-creation'}} @@ -75,14 +76,15 @@ jobs: --insecure \ --module-config-file ./module-config.yaml \ --version $MODULE_TEMPLATE_VERSION -v \ - --sec-scanners-config ./template-operator/sec-scanners-config.yaml \ + --sec-scanners-config sec-scanners-config.yaml \ --output /tmp/module-config-template.yaml echo "MODULE_TEMPLATE_PATH=/tmp/module-config-template.yaml" >> "$GITHUB_ENV" - name: Create a different security scanners config file for different layers if: ${{matrix.e2e-test == 'test-same-version-module-creation'}} run: | cd ./template-operator - echo 'module-name: template-operator + echo \ + "module-name: template-operator rc-tag: 0.5.0 dev-branch: main protecode: @@ -90,8 +92,9 @@ jobs: whitesource: language: golang-mod exclude: - - "**/test/**" - - "**/*_test.go"' > sec-scanners-config-changed.yaml + - \"**/test/**\" + - \"**/*_test.go\"" > sec-scanners-config-changed.yaml + cat sec-scanners-config-changed.yaml - name: Verify module template if: ${{ matrix.e2e-test == 'test-moduleconfig-module-creation' || matrix.e2e-test == 'test-kubebuilder-module-creation'}} run: | diff --git a/internal/cli/logs.go b/internal/cli/logs.go index f9a003ed7..adf4050c0 100644 --- a/internal/cli/logs.go +++ b/internal/cli/logs.go @@ -9,7 +9,6 @@ import ( "go.uber.org/zap" "go.uber.org/zap/zapcore" "k8s.io/klog/v2" - ctrllog "sigs.k8s.io/controller-runtime/pkg/log" ) // NewLogger returns the logger used for CLI log output (used in Hydroform deployments) @@ -26,7 +25,6 @@ func NewLogger(printLogs bool) *zap.Logger { logger = zap.NewNop() } logr := zapr.NewLoggerWithOptions(logger) - ctrllog.SetLogger(zapr.NewLoggerWithOptions(zap.NewNop())) klog.SetLogger(logr) ocm.DefaultContext().LoggingContext().SetBaseLogger(logr) ocm.DefaultContext().LoggingContext().SetDefaultLevel(9) diff --git a/pkg/module/remote.go b/pkg/module/remote.go index 105c541fb..693da5d1b 100644 --- a/pkg/module/remote.go +++ b/pkg/module/remote.go @@ -151,11 +151,10 @@ func (r *Remote) Push(archive *comparch.ComponentArchive, overwrite bool) (ocm.C if descriptorResourcesAreEquivalent(archive.GetDescriptor().Resources, versionAccess.GetDescriptor().Resources) { return versionAccess, false, nil - } else { - return nil, false, fmt.Errorf("version %s already exists with different content, please use "+ - "--module-archive-version-overwrite flag to overwrite it", - archive.ComponentVersionAccess.GetVersion()) } + return nil, false, fmt.Errorf("version %s already exists with different content, please use "+ + "--module-archive-version-overwrite flag to overwrite it", + archive.ComponentVersionAccess.GetVersion()) } } @@ -201,15 +200,26 @@ func descriptorResourcesAreEquivalent(localResources, remoteResources compdesc.R for _, res := range remoteResources { localResource := localResourcesMap[res.Name] if res.Name == RawManifestLayerName { - remoteAccessObject := res.Access.(*runtime.UnstructuredVersionedTypedObject).Object - _, ok := localResourcesMap[res.Name] + remoteAccess, ok := res.Access.(*runtime.UnstructuredVersionedTypedObject) if !ok { return false } - localAccessObject := localResource.Access.(*localblob.AccessSpec) + _, ok = localResourcesMap[res.Name] + if !ok { + return false + } + localAccessObject, ok := localResource.Access.(*localblob.AccessSpec) + if !ok { + return false + } + + remoteAccessLocalReference, ok := remoteAccess.Object[accessLocalReferenceFieldName].(string) + if !ok { + return false + } // Trimming 7 characters because locally the sha256 is followed by '.' but remote it is followed by ':' - if remoteAccessObject[accessLocalReferenceFieldName].(string)[7:] != localAccessObject.LocalReference[7:] { + if remoteAccessLocalReference[7:] != localAccessObject.LocalReference[7:] { return false } } else if !res.IsEquivalent(&localResource) { diff --git a/tests/e2e/create_module/kyma_create_module_same_version_test.go b/tests/e2e/create_module/kyma_create_module_same_version_test.go index 1ec62d966..c4df39252 100644 --- a/tests/e2e/create_module/kyma_create_module_same_version_test.go +++ b/tests/e2e/create_module/kyma_create_module_same_version_test.go @@ -1,6 +1,7 @@ package create_module_test import ( + "os" "testing" "github.com/stretchr/testify/assert" @@ -8,13 +9,18 @@ import ( "github.com/kyma-project/cli/tests/e2e" ) +const ( + ociRepositoryEnvVar = "OCI_REPOSITORY_URL" + moduleTemplateVersionEnvVar = "MODULE_TEMPLATE_VERSION" +) + func Test_SameVersion_ModuleCreation(t *testing.T) { path := "../../../template-operator" - registry := "http://k3d-oci.localhost:5001" configFilePath := "../../../template-operator/module-config.yaml" secScannerConfigFile := "../../../template-operator/sec-scanners-config.yaml" - changedsecScannerConfigFile := "../../../template-operator/sec-scanners-config-changed.yaml" - version := "1.0.0" + changedSecScannerConfigFile := "../../../template-operator/sec-scanners-config-changed.yaml" + version := os.Getenv(moduleTemplateVersionEnvVar) + registry := os.Getenv(ociRepositoryEnvVar) t.Run("Create same version module with module-archive-version-overwrite flag", func(t *testing.T) { err := e2e.CreateModuleCommand(true, path, registry, configFilePath, version, secScannerConfigFile) @@ -27,9 +33,9 @@ func Test_SameVersion_ModuleCreation(t *testing.T) { assert.Nil(t, err) }) - t.Run("Create same version module but different content without module-archive-version-overwrite flag", + t.Run("Create same version module, but different content without module-archive-version-overwrite flag", func(t *testing.T) { - err := e2e.CreateModuleCommand(false, path, registry, configFilePath, version, changedsecScannerConfigFile) + err := e2e.CreateModuleCommand(false, path, registry, configFilePath, version, changedSecScannerConfigFile) assert.Equal(t, e2e.ErrCreateModuleFailedWithSameVersion, err) }) } diff --git a/tests/e2e/create_module/kyma_create_module_test.go b/tests/e2e/create_module/kyma_create_module_test.go index d4aef33d6..9db93e50d 100644 --- a/tests/e2e/create_module/kyma_create_module_test.go +++ b/tests/e2e/create_module/kyma_create_module_test.go @@ -49,10 +49,16 @@ func Test_ModuleTemplate(t *testing.T) { assert.Equal(t, 2, len(descriptor.Resources)) resource := descriptor.Resources[0] + assert.Equal(t, "template-operator", resource.Name) + assert.Equal(t, ocmMetaV1.ExternalRelation, resource.Relation) + assert.Equal(t, "ociImage", resource.Type) + expectedModuleTemplateVersion := os.Getenv("MODULE_TEMPLATE_VERSION") + assert.Equal(t, expectedModuleTemplateVersion, resource.Version) + + resource = descriptor.Resources[1] assert.Equal(t, module.RawManifestLayerName, resource.Name) assert.Equal(t, ocmMetaV1.LocalRelation, resource.Relation) assert.Equal(t, module.TypeYaml, resource.Type) - expectedModuleTemplateVersion := os.Getenv("MODULE_TEMPLATE_VERSION") assert.Equal(t, expectedModuleTemplateVersion, resource.Version) }) diff --git a/tests/e2e/util.go b/tests/e2e/util.go index 093ed1dab..72bc1f883 100644 --- a/tests/e2e/util.go +++ b/tests/e2e/util.go @@ -216,11 +216,11 @@ func CreateModuleCommand(versionOverwrite bool, if versionOverwrite { createModuleCmd = exec.Command("kyma", "alpha", "create", "module", "--path", path, "--registry", registry, "--insecure", "--module-config-file", configFilePath, - "--version", version, "--module-archive-version-overwrite", "--sec-scanner-config", secScannerConfig) + "--version", version, "--module-archive-version-overwrite", "--sec-scanners-config", secScannerConfig) } else { createModuleCmd = exec.Command("kyma", "alpha", "create", "module", "--path", path, "--registry", registry, "--insecure", "--module-config-file", configFilePath, - "--version", version, "--sec-scanner-config", secScannerConfig) + "--version", version, "--sec-scanners-config", secScannerConfig) } createOut, err := createModuleCmd.CombinedOutput() From 0a3afb3abdbd5ec2263c5b43e349ecea06beabf8 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Mon, 27 Nov 2023 16:01:52 +0100 Subject: [PATCH 5/9] Add OCI interface --- pkg/module/oci_repo.go | 47 ++++++++++++++++++++++++++++++++++++++++++ pkg/module/remote.go | 27 ++++++------------------ 2 files changed, 53 insertions(+), 21 deletions(-) create mode 100644 pkg/module/oci_repo.go diff --git a/pkg/module/oci_repo.go b/pkg/module/oci_repo.go new file mode 100644 index 000000000..cc39c7136 --- /dev/null +++ b/pkg/module/oci_repo.go @@ -0,0 +1,47 @@ +package module + +import ( + "fmt" + + "github.com/open-component-model/ocm/pkg/common" + "github.com/open-component-model/ocm/pkg/contexts/ocm" + "github.com/open-component-model/ocm/pkg/contexts/ocm/cpi" + "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" + "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer" + "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer/transferhandler/standard" +) + +//go:generate mockery --name OciRepo +type OciRepoImpl interface { + ComponentVersionExists(archive *comparch.ComponentArchive, repo cpi.Repository) (bool, error) + GetComponentVersion(archive *comparch.ComponentArchive, repo cpi.Repository) (ocm.ComponentVersionAccess, error) + PushComponentVersion(archive *comparch.ComponentArchive, repository cpi.Repository, overwrite bool) error +} + +type OciRepo struct{} + +func (r *OciRepo) ComponentVersionExists(archive *comparch.ComponentArchive, repo cpi.Repository) (bool, error) { + return repo.ExistsComponentVersion(archive.ComponentVersionAccess.GetName(), + archive.ComponentVersionAccess.GetVersion()) +} + +func (r *OciRepo) GetComponentVersion(archive *comparch.ComponentArchive, + repo cpi.Repository) (ocm.ComponentVersionAccess, error) { + return repo.LookupComponentVersion(archive.ComponentVersionAccess.GetName(), + archive.ComponentVersionAccess.GetVersion()) +} + +func (r *OciRepo) PushComponentVersion(archive *comparch.ComponentArchive, repo cpi.Repository, overwrite bool) error { + transferHandler, err := standard.New(standard.Overwrite(overwrite)) + if err != nil { + return fmt.Errorf("could not setup archive transfer: %w", err) + } + + if err = transfer.TransferVersion( + common.NewLoggingPrinter(archive.GetContext().Logger()), nil, archive.ComponentVersionAccess, repo, + &customTransferHandler{transferHandler}, + ); err != nil { + return fmt.Errorf("could not finish component transfer: %w", err) + } + return nil +} diff --git a/pkg/module/remote.go b/pkg/module/remote.go index 693da5d1b..eddce2f14 100644 --- a/pkg/module/remote.go +++ b/pkg/module/remote.go @@ -7,7 +7,6 @@ import ( "regexp" "strings" - "github.com/open-component-model/ocm/pkg/common" "github.com/open-component-model/ocm/pkg/contexts/credentials" "github.com/open-component-model/ocm/pkg/contexts/credentials/repositories/dockerconfig" oci "github.com/open-component-model/ocm/pkg/contexts/oci/repositories/ocireg" @@ -18,9 +17,7 @@ import ( "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/genericocireg" "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/ocireg" - "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer" "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer/transferhandler" - "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer/transferhandler/standard" "github.com/open-component-model/ocm/pkg/runtime" ) @@ -39,6 +36,7 @@ type Remote struct { Credentials string Token string Insecure bool + OciRepo } func (r *Remote) GetRepository(ctx cpi.Context) (cpi.Repository, error) { @@ -137,13 +135,10 @@ func (r *Remote) Push(archive *comparch.ComponentArchive, overwrite bool) (ocm.C } if !overwrite { - versionExists, _ := repo.ExistsComponentVersion(archive.ComponentVersionAccess.GetName(), - archive.ComponentVersionAccess.GetVersion()) + versionExists, _ := r.ComponentVersionExists(archive, repo) if versionExists { - versionAccess, err := repo.LookupComponentVersion( - archive.ComponentVersionAccess.GetName(), archive.ComponentVersionAccess.GetVersion(), - ) + versionAccess, err := r.GetComponentVersion(archive, repo) if err != nil { return nil, false, fmt.Errorf("could not lookup component version: %w", err) } @@ -158,21 +153,11 @@ func (r *Remote) Push(archive *comparch.ComponentArchive, overwrite bool) (ocm.C } } - transferHandler, err := standard.New(standard.Overwrite(overwrite)) - if err != nil { - return nil, false, fmt.Errorf("could not setup archive transfer: %w", err) - } - - if err = transfer.TransferVersion( - common.NewLoggingPrinter(archive.GetContext().Logger()), nil, archive.ComponentVersionAccess, repo, - &customTransferHandler{transferHandler}, - ); err != nil { - return nil, false, fmt.Errorf("could not finish component transfer: %w", err) + if err = r.PushComponentVersion(archive, repo, overwrite); err != nil { + return nil, false, err } - componentVersion, err := repo.LookupComponentVersion( - archive.ComponentVersionAccess.GetName(), archive.ComponentVersionAccess.GetVersion(), - ) + componentVersion, err := r.GetComponentVersion(archive, repo) return componentVersion, err == nil, err } From 06b2602a18d54622ea7dfee6b6a9085470074bb6 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Tue, 28 Nov 2023 10:34:40 +0100 Subject: [PATCH 6/9] Start unit test --- internal/kube/mocks/KymaKube.go | 75 +++++++++++++++++++-- pkg/module/mocks/OciRepoAccess.go | 105 ++++++++++++++++++++++++++++++ pkg/module/oci_repo.go | 4 +- pkg/module/remote.go | 2 +- pkg/module/remote_test.go | 83 +++++++++++++++++++++++ 5 files changed, 260 insertions(+), 9 deletions(-) create mode 100644 pkg/module/mocks/OciRepoAccess.go diff --git a/internal/kube/mocks/KymaKube.go b/internal/kube/mocks/KymaKube.go index b91dbf5e0..e72a15b2d 100644 --- a/internal/kube/mocks/KymaKube.go +++ b/internal/kube/mocks/KymaKube.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.20.0. DO NOT EDIT. +// Code generated by mockery v2.38.0. DO NOT EDIT. package mocks @@ -43,6 +43,10 @@ func (_m *KymaKube) Apply(ctx context.Context, force bool, objs ...client.Object _ca = append(_ca, _va...) ret := _m.Called(_ca...) + if len(ret) == 0 { + panic("no return value specified for Apply") + } + var r0 error if rf, ok := ret.Get(0).(func(context.Context, bool, ...client.Object) error); ok { r0 = rf(ctx, force, objs...) @@ -57,6 +61,10 @@ func (_m *KymaKube) Apply(ctx context.Context, force bool, objs ...client.Object func (_m *KymaKube) Ctrl() client.WithWatch { ret := _m.Called() + if len(ret) == 0 { + panic("no return value specified for Ctrl") + } + var r0 client.WithWatch if rf, ok := ret.Get(0).(func() client.WithWatch); ok { r0 = rf() @@ -73,6 +81,10 @@ func (_m *KymaKube) Ctrl() client.WithWatch { func (_m *KymaKube) DefaultNamespace() string { ret := _m.Called() + if len(ret) == 0 { + panic("no return value specified for DefaultNamespace") + } + var r0 string if rf, ok := ret.Get(0).(func() string); ok { r0 = rf() @@ -87,6 +99,10 @@ func (_m *KymaKube) DefaultNamespace() string { func (_m *KymaKube) Dynamic() dynamic.Interface { ret := _m.Called() + if len(ret) == 0 { + panic("no return value specified for Dynamic") + } + var r0 dynamic.Interface if rf, ok := ret.Get(0).(func() dynamic.Interface); ok { r0 = rf() @@ -103,6 +119,10 @@ func (_m *KymaKube) Dynamic() dynamic.Interface { func (_m *KymaKube) IsPodDeployed(namespace string, name string) (bool, error) { ret := _m.Called(namespace, name) + if len(ret) == 0 { + panic("no return value specified for IsPodDeployed") + } + var r0 bool var r1 error if rf, ok := ret.Get(0).(func(string, string) (bool, error)); ok { @@ -127,6 +147,10 @@ func (_m *KymaKube) IsPodDeployed(namespace string, name string) (bool, error) { func (_m *KymaKube) IsPodDeployedByLabel(namespace string, labelName string, labelValue string) (bool, error) { ret := _m.Called(namespace, labelName, labelValue) + if len(ret) == 0 { + panic("no return value specified for IsPodDeployedByLabel") + } + var r0 bool var r1 error if rf, ok := ret.Get(0).(func(string, string, string) (bool, error)); ok { @@ -151,6 +175,10 @@ func (_m *KymaKube) IsPodDeployedByLabel(namespace string, labelName string, lab func (_m *KymaKube) Istio() versioned.Interface { ret := _m.Called() + if len(ret) == 0 { + panic("no return value specified for Istio") + } + var r0 versioned.Interface if rf, ok := ret.Get(0).(func() versioned.Interface); ok { r0 = rf() @@ -167,6 +195,10 @@ func (_m *KymaKube) Istio() versioned.Interface { func (_m *KymaKube) KubeConfig() *api.Config { ret := _m.Called() + if len(ret) == 0 { + panic("no return value specified for KubeConfig") + } + var r0 *api.Config if rf, ok := ret.Get(0).(func() *api.Config); ok { r0 = rf() @@ -183,6 +215,10 @@ func (_m *KymaKube) KubeConfig() *api.Config { func (_m *KymaKube) ParseManifest(manifest []byte) ([]client.Object, error) { ret := _m.Called(manifest) + if len(ret) == 0 { + panic("no return value specified for ParseManifest") + } + var r0 []client.Object var r1 error if rf, ok := ret.Get(0).(func([]byte) ([]client.Object, error)); ok { @@ -209,6 +245,10 @@ func (_m *KymaKube) ParseManifest(manifest []byte) ([]client.Object, error) { func (_m *KymaKube) RestConfig() *rest.Config { ret := _m.Called() + if len(ret) == 0 { + panic("no return value specified for RestConfig") + } + var r0 *rest.Config if rf, ok := ret.Get(0).(func() *rest.Config); ok { r0 = rf() @@ -225,6 +265,10 @@ func (_m *KymaKube) RestConfig() *rest.Config { func (_m *KymaKube) Static() kubernetes.Interface { ret := _m.Called() + if len(ret) == 0 { + panic("no return value specified for Static") + } + var r0 kubernetes.Interface if rf, ok := ret.Get(0).(func() kubernetes.Interface); ok { r0 = rf() @@ -241,6 +285,10 @@ func (_m *KymaKube) Static() kubernetes.Interface { func (_m *KymaKube) WaitDeploymentStatus(namespace string, name string, cond v1.DeploymentConditionType, status corev1.ConditionStatus) error { ret := _m.Called(namespace, name, cond, status) + if len(ret) == 0 { + panic("no return value specified for WaitDeploymentStatus") + } + var r0 error if rf, ok := ret.Get(0).(func(string, string, v1.DeploymentConditionType, corev1.ConditionStatus) error); ok { r0 = rf(namespace, name, cond, status) @@ -255,6 +303,10 @@ func (_m *KymaKube) WaitDeploymentStatus(namespace string, name string, cond v1. func (_m *KymaKube) WaitPodStatus(namespace string, name string, status corev1.PodPhase) error { ret := _m.Called(namespace, name, status) + if len(ret) == 0 { + panic("no return value specified for WaitPodStatus") + } + var r0 error if rf, ok := ret.Get(0).(func(string, string, corev1.PodPhase) error); ok { r0 = rf(namespace, name, status) @@ -269,6 +321,10 @@ func (_m *KymaKube) WaitPodStatus(namespace string, name string, status corev1.P func (_m *KymaKube) WaitPodStatusByLabel(namespace string, labelName string, labelValue string, status corev1.PodPhase) error { ret := _m.Called(namespace, labelName, labelValue, status) + if len(ret) == 0 { + panic("no return value specified for WaitPodStatusByLabel") + } + var r0 error if rf, ok := ret.Get(0).(func(string, string, string, corev1.PodPhase) error); ok { r0 = rf(namespace, labelName, labelValue, status) @@ -283,6 +339,10 @@ func (_m *KymaKube) WaitPodStatusByLabel(namespace string, labelName string, lab func (_m *KymaKube) WatchObject(ctx context.Context, obj client.Object, checkFn func(client.Object) (bool, error)) error { ret := _m.Called(ctx, obj, checkFn) + if len(ret) == 0 { + panic("no return value specified for WatchObject") + } + var r0 error if rf, ok := ret.Get(0).(func(context.Context, client.Object, func(client.Object) (bool, error)) error); ok { r0 = rf(ctx, obj, checkFn) @@ -297,6 +357,10 @@ func (_m *KymaKube) WatchObject(ctx context.Context, obj client.Object, checkFn func (_m *KymaKube) WatchResource(res schema.GroupVersionResource, name string, namespace string, checkFn func(*unstructured.Unstructured) (bool, error)) error { ret := _m.Called(res, name, namespace, checkFn) + if len(ret) == 0 { + panic("no return value specified for WatchResource") + } + var r0 error if rf, ok := ret.Get(0).(func(schema.GroupVersionResource, string, string, func(*unstructured.Unstructured) (bool, error)) error); ok { r0 = rf(res, name, namespace, checkFn) @@ -307,13 +371,12 @@ func (_m *KymaKube) WatchResource(res schema.GroupVersionResource, name string, return r0 } -type mockConstructorTestingTNewKymaKube interface { +// NewKymaKube creates a new instance of KymaKube. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewKymaKube(t interface { mock.TestingT Cleanup(func()) -} - -// NewKymaKube creates a new instance of KymaKube. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. -func NewKymaKube(t mockConstructorTestingTNewKymaKube) *KymaKube { +}) *KymaKube { mock := &KymaKube{} mock.Mock.Test(t) diff --git a/pkg/module/mocks/OciRepoAccess.go b/pkg/module/mocks/OciRepoAccess.go new file mode 100644 index 000000000..cec1faeec --- /dev/null +++ b/pkg/module/mocks/OciRepoAccess.go @@ -0,0 +1,105 @@ +// Code generated by mockery v2.38.0. DO NOT EDIT. + +package mocks + +import ( + internal "github.com/open-component-model/ocm/pkg/contexts/ocm/internal" + comparch "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" + + mock "github.com/stretchr/testify/mock" +) + +// OciRepoAccess is an autogenerated mock type for the OciRepoAccess type +type OciRepoAccess struct { + mock.Mock +} + +// ComponentVersionExists provides a mock function with given fields: archive, repo +func (_m *OciRepoAccess) ComponentVersionExists(archive *comparch.ComponentArchive, repo internal.Repository) (bool, error) { + ret := _m.Called(archive, repo) + + if len(ret) == 0 { + panic("no return value specified for ComponentVersionExists") + } + + var r0 bool + var r1 error + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository) (bool, error)); ok { + return rf(archive, repo) + } + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository) bool); ok { + r0 = rf(archive, repo) + } else { + r0 = ret.Get(0).(bool) + } + + if rf, ok := ret.Get(1).(func(*comparch.ComponentArchive, internal.Repository) error); ok { + r1 = rf(archive, repo) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetComponentVersion provides a mock function with given fields: archive, repo +func (_m *OciRepoAccess) GetComponentVersion(archive *comparch.ComponentArchive, repo internal.Repository) (internal.ComponentVersionAccess, error) { + ret := _m.Called(archive, repo) + + if len(ret) == 0 { + panic("no return value specified for GetComponentVersion") + } + + var r0 internal.ComponentVersionAccess + var r1 error + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository) (internal.ComponentVersionAccess, error)); ok { + return rf(archive, repo) + } + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository) internal.ComponentVersionAccess); ok { + r0 = rf(archive, repo) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(internal.ComponentVersionAccess) + } + } + + if rf, ok := ret.Get(1).(func(*comparch.ComponentArchive, internal.Repository) error); ok { + r1 = rf(archive, repo) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// PushComponentVersion provides a mock function with given fields: archive, repository, overwrite +func (_m *OciRepoAccess) PushComponentVersion(archive *comparch.ComponentArchive, repository internal.Repository, overwrite bool) error { + ret := _m.Called(archive, repository, overwrite) + + if len(ret) == 0 { + panic("no return value specified for PushComponentVersion") + } + + var r0 error + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository, bool) error); ok { + r0 = rf(archive, repository, overwrite) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// NewOciRepoAccess creates a new instance of OciRepoAccess. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewOciRepoAccess(t interface { + mock.TestingT + Cleanup(func()) +}) *OciRepoAccess { + mock := &OciRepoAccess{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/module/oci_repo.go b/pkg/module/oci_repo.go index cc39c7136..ebf5a1c7d 100644 --- a/pkg/module/oci_repo.go +++ b/pkg/module/oci_repo.go @@ -11,8 +11,8 @@ import ( "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer/transferhandler/standard" ) -//go:generate mockery --name OciRepo -type OciRepoImpl interface { +//go:generate mockery --name OciRepoAccess +type OciRepoAccess interface { ComponentVersionExists(archive *comparch.ComponentArchive, repo cpi.Repository) (bool, error) GetComponentVersion(archive *comparch.ComponentArchive, repo cpi.Repository) (ocm.ComponentVersionAccess, error) PushComponentVersion(archive *comparch.ComponentArchive, repository cpi.Repository, overwrite bool) error diff --git a/pkg/module/remote.go b/pkg/module/remote.go index eddce2f14..931edc2b9 100644 --- a/pkg/module/remote.go +++ b/pkg/module/remote.go @@ -36,7 +36,7 @@ type Remote struct { Credentials string Token string Insecure bool - OciRepo + OciRepoAccess } func (r *Remote) GetRepository(ctx cpi.Context) (cpi.Repository, error) { diff --git a/pkg/module/remote_test.go b/pkg/module/remote_test.go index 400d04b73..2023e6728 100644 --- a/pkg/module/remote_test.go +++ b/pkg/module/remote_test.go @@ -1,9 +1,19 @@ package module_test import ( + "fmt" "testing" + "github.com/mandelsoft/vfs/pkg/memoryfs" + "github.com/open-component-model/ocm/pkg/contexts/ocm" + "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" + ocmv1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1" + "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/kyma-project/cli/pkg/module" + "github.com/kyma-project/cli/pkg/module/mocks" ) func TestNoSchemaURL(t *testing.T) { @@ -38,3 +48,76 @@ func TestNoSchemaURL(t *testing.T) { ) } } + +func TestRemote_Push(t *testing.T) { + archiveFS := memoryfs.New() + cd := &compdesc.ComponentDescriptor{} + cd.ComponentSpec.SetName("test.io/module/test") + cd.ComponentSpec.SetVersion("1.0.0") + cd.Metadata.ConfiguredVersion = "v2" + builtByCLI, _ := ocmv1.NewLabel("kyma-project.io/built-by", "cli", ocmv1.WithVersion("v1")) + cd.Provider = ocmv1.Provider{Name: "kyma-project.io", Labels: ocmv1.Labels{*builtByCLI}} + compdesc.DefaultResources(cd) + + archive, _ := module.CreateArchive(archiveFS, "./mod", cd) + ociRepoAccessMock := mocks.OciRepoAccess{} + ociRepoAccessMock.On("PushComponentVersion", archive, mock.Anything, mock.Anything).Return(nil) + ociRepoAccessMock.On("GetComponentVersion", archive, + mock.Anything).Return(mock.AnythingOfType("internal.ComponentVersionAccess")) + + type fields struct { + OciRepoAccess module.OciRepoAccess + } + type args struct { + archive *comparch.ComponentArchive + overwrite bool + } + tests := []struct { + name string + fields fields + args args + want ocm.ComponentVersionAccess + want1 bool + wantErr assert.ErrorAssertionFunc + }{ + { + name: "Same version, same content, with overwrite flag", + args: args{ + overwrite: true, + archive: archive, + }, + fields: fields{ + OciRepoAccess: &ociRepoAccessMock, + }, + }, + { + name: "Same version, same content, without overwrite flag", + }, + { + name: "Same version, different content, with overwrite flag", + }, + { + name: "Same version, different content, without overwrite flag", + }, + { + name: "different version, with overwrite flag", + }, + { + name: "different version, without overwrite flag", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &module.Remote{ + Insecure: true, + OciRepoAccess: tt.fields.OciRepoAccess, + } + got, got1, err := r.Push(tt.args.archive, tt.args.overwrite) + if !tt.wantErr(t, err, fmt.Sprintf("Push(%v, %v)", tt.args.archive, tt.args.overwrite)) { + return + } + assert.Equalf(t, tt.want, got, "Push(%v, %v)", tt.args.archive, tt.args.overwrite) + assert.Equalf(t, tt.want1, got1, "Push(%v, %v)", tt.args.archive, tt.args.overwrite) + }) + } +} From cffecbf0ffa794fa71c42aebf55df078898a1def Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Tue, 28 Nov 2023 13:27:44 +0100 Subject: [PATCH 7/9] Fix mock --- pkg/module/mocks/OciRepoAccess.go | 2 +- pkg/module/oci_repo.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/module/mocks/OciRepoAccess.go b/pkg/module/mocks/OciRepoAccess.go index cec1faeec..19d4855b4 100644 --- a/pkg/module/mocks/OciRepoAccess.go +++ b/pkg/module/mocks/OciRepoAccess.go @@ -3,7 +3,7 @@ package mocks import ( - internal "github.com/open-component-model/ocm/pkg/contexts/ocm/internal" + internal "github.com/open-component-model/ocm/pkg/contexts/ocm/cpi" comparch "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" mock "github.com/stretchr/testify/mock" diff --git a/pkg/module/oci_repo.go b/pkg/module/oci_repo.go index ebf5a1c7d..207ee0b75 100644 --- a/pkg/module/oci_repo.go +++ b/pkg/module/oci_repo.go @@ -11,7 +11,7 @@ import ( "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer/transferhandler/standard" ) -//go:generate mockery --name OciRepoAccess +//go:generate mockery --name OciRepoAccess --replace-type github.com/open-component-model/ocm/pkg/contexts/ocm/internal=internal:github.com/open-component-model/ocm/pkg/contexts/ocm/cpi type OciRepoAccess interface { ComponentVersionExists(archive *comparch.ComponentArchive, repo cpi.Repository) (bool, error) GetComponentVersion(archive *comparch.ComponentArchive, repo cpi.Repository) (ocm.ComponentVersionAccess, error) From cb20c585d98d44aa48901cb48a968ce74190d3d2 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Tue, 28 Nov 2023 14:45:37 +0100 Subject: [PATCH 8/9] Unit test --- cmd/kyma/alpha/create/module/module.go | 11 ++- cmd/kyma/alpha/verify/module/module.go | 14 +-- pkg/module/mocks/OciRepoAccess.go | 46 ++++++--- pkg/module/oci_repo.go | 52 ++++++++++- pkg/module/remote.go | 47 +--------- pkg/module/remote_test.go | 123 ++++++++++++++++++++----- 6 files changed, 197 insertions(+), 96 deletions(-) diff --git a/cmd/kyma/alpha/create/module/module.go b/cmd/kyma/alpha/create/module/module.go index b7a7cd9b0..5f4580232 100644 --- a/cmd/kyma/alpha/create/module/module.go +++ b/cmd/kyma/alpha/create/module/module.go @@ -479,11 +479,12 @@ func (cmd *command) validateDefaultCR(ctx context.Context, modDef *module.Defini func (cmd *command) getRemote(nameMapping module.NameMapping) (*module.Remote, error) { res := &module.Remote{ - Registry: cmd.opts.RegistryURL, - NameMapping: nameMapping, - Credentials: cmd.opts.Credentials, - Token: cmd.opts.Token, - Insecure: cmd.opts.Insecure, + Registry: cmd.opts.RegistryURL, + NameMapping: nameMapping, + Credentials: cmd.opts.Credentials, + Token: cmd.opts.Token, + Insecure: cmd.opts.Insecure, + OciRepoAccess: &module.OciRepo{}, } if strings.HasPrefix(strings.ToLower(cmd.opts.RegistryURL), "https:") { diff --git a/cmd/kyma/alpha/verify/module/module.go b/cmd/kyma/alpha/verify/module/module.go index 71560b1af..2363ec91b 100644 --- a/cmd/kyma/alpha/verify/module/module.go +++ b/cmd/kyma/alpha/verify/module/module.go @@ -1,9 +1,10 @@ package module import ( + "github.com/spf13/cobra" + "github.com/kyma-project/cli/internal/cli" "github.com/kyma-project/cli/pkg/module" - "github.com/spf13/cobra" ) type command struct { @@ -80,11 +81,12 @@ func (c *command) Run(_ []string) error { } remote := &module.Remote{ - Registry: c.opts.RegistryURL, - NameMapping: nameMappingMode, - Credentials: c.opts.Credentials, - Token: c.opts.Token, - Insecure: c.opts.Insecure, + Registry: c.opts.RegistryURL, + NameMapping: nameMappingMode, + Credentials: c.opts.Credentials, + Token: c.opts.Token, + Insecure: c.opts.Insecure, + OciRepoAccess: &module.OciRepo{}, } if err := module.Verify(signCfg, remote); err != nil { diff --git a/pkg/module/mocks/OciRepoAccess.go b/pkg/module/mocks/OciRepoAccess.go index 19d4855b4..b43be64ba 100644 --- a/pkg/module/mocks/OciRepoAccess.go +++ b/pkg/module/mocks/OciRepoAccess.go @@ -3,10 +3,10 @@ package mocks import ( - internal "github.com/open-component-model/ocm/pkg/contexts/ocm/cpi" comparch "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" - mock "github.com/stretchr/testify/mock" + + ocm "github.com/open-component-model/ocm/pkg/contexts/ocm" ) // OciRepoAccess is an autogenerated mock type for the OciRepoAccess type @@ -15,7 +15,7 @@ type OciRepoAccess struct { } // ComponentVersionExists provides a mock function with given fields: archive, repo -func (_m *OciRepoAccess) ComponentVersionExists(archive *comparch.ComponentArchive, repo internal.Repository) (bool, error) { +func (_m *OciRepoAccess) ComponentVersionExists(archive *comparch.ComponentArchive, repo ocm.Repository) (bool, error) { ret := _m.Called(archive, repo) if len(ret) == 0 { @@ -24,16 +24,16 @@ func (_m *OciRepoAccess) ComponentVersionExists(archive *comparch.ComponentArchi var r0 bool var r1 error - if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository) (bool, error)); ok { + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, ocm.Repository) (bool, error)); ok { return rf(archive, repo) } - if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository) bool); ok { + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, ocm.Repository) bool); ok { r0 = rf(archive, repo) } else { r0 = ret.Get(0).(bool) } - if rf, ok := ret.Get(1).(func(*comparch.ComponentArchive, internal.Repository) error); ok { + if rf, ok := ret.Get(1).(func(*comparch.ComponentArchive, ocm.Repository) error); ok { r1 = rf(archive, repo) } else { r1 = ret.Error(1) @@ -42,28 +42,46 @@ func (_m *OciRepoAccess) ComponentVersionExists(archive *comparch.ComponentArchi return r0, r1 } +// DescriptorResourcesAreEquivalent provides a mock function with given fields: archive, remoteVersion +func (_m *OciRepoAccess) DescriptorResourcesAreEquivalent(archive *comparch.ComponentArchive, remoteVersion ocm.ComponentVersionAccess) bool { + ret := _m.Called(archive, remoteVersion) + + if len(ret) == 0 { + panic("no return value specified for DescriptorResourcesAreEquivalent") + } + + var r0 bool + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, ocm.ComponentVersionAccess) bool); ok { + r0 = rf(archive, remoteVersion) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + // GetComponentVersion provides a mock function with given fields: archive, repo -func (_m *OciRepoAccess) GetComponentVersion(archive *comparch.ComponentArchive, repo internal.Repository) (internal.ComponentVersionAccess, error) { +func (_m *OciRepoAccess) GetComponentVersion(archive *comparch.ComponentArchive, repo ocm.Repository) (ocm.ComponentVersionAccess, error) { ret := _m.Called(archive, repo) if len(ret) == 0 { panic("no return value specified for GetComponentVersion") } - var r0 internal.ComponentVersionAccess + var r0 ocm.ComponentVersionAccess var r1 error - if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository) (internal.ComponentVersionAccess, error)); ok { + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, ocm.Repository) (ocm.ComponentVersionAccess, error)); ok { return rf(archive, repo) } - if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository) internal.ComponentVersionAccess); ok { + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, ocm.Repository) ocm.ComponentVersionAccess); ok { r0 = rf(archive, repo) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(internal.ComponentVersionAccess) + r0 = ret.Get(0).(ocm.ComponentVersionAccess) } } - if rf, ok := ret.Get(1).(func(*comparch.ComponentArchive, internal.Repository) error); ok { + if rf, ok := ret.Get(1).(func(*comparch.ComponentArchive, ocm.Repository) error); ok { r1 = rf(archive, repo) } else { r1 = ret.Error(1) @@ -73,7 +91,7 @@ func (_m *OciRepoAccess) GetComponentVersion(archive *comparch.ComponentArchive, } // PushComponentVersion provides a mock function with given fields: archive, repository, overwrite -func (_m *OciRepoAccess) PushComponentVersion(archive *comparch.ComponentArchive, repository internal.Repository, overwrite bool) error { +func (_m *OciRepoAccess) PushComponentVersion(archive *comparch.ComponentArchive, repository ocm.Repository, overwrite bool) error { ret := _m.Called(archive, repository, overwrite) if len(ret) == 0 { @@ -81,7 +99,7 @@ func (_m *OciRepoAccess) PushComponentVersion(archive *comparch.ComponentArchive } var r0 error - if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, internal.Repository, bool) error); ok { + if rf, ok := ret.Get(0).(func(*comparch.ComponentArchive, ocm.Repository, bool) error); ok { r0 = rf(archive, repository, overwrite) } else { r0 = ret.Error(0) diff --git a/pkg/module/oci_repo.go b/pkg/module/oci_repo.go index 207ee0b75..035eef614 100644 --- a/pkg/module/oci_repo.go +++ b/pkg/module/oci_repo.go @@ -5,17 +5,21 @@ import ( "github.com/open-component-model/ocm/pkg/common" "github.com/open-component-model/ocm/pkg/contexts/ocm" + "github.com/open-component-model/ocm/pkg/contexts/ocm/accessmethods/localblob" + "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" "github.com/open-component-model/ocm/pkg/contexts/ocm/cpi" "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer" "github.com/open-component-model/ocm/pkg/contexts/ocm/transfer/transferhandler/standard" + "github.com/open-component-model/ocm/pkg/runtime" ) -//go:generate mockery --name OciRepoAccess --replace-type github.com/open-component-model/ocm/pkg/contexts/ocm/internal=internal:github.com/open-component-model/ocm/pkg/contexts/ocm/cpi +//go:generate mockery --name OciRepoAccess --replace-type github.com/open-component-model/ocm/pkg/contexts/ocm/internal=ocm:github.com/open-component-model/ocm/pkg/contexts/ocm type OciRepoAccess interface { ComponentVersionExists(archive *comparch.ComponentArchive, repo cpi.Repository) (bool, error) GetComponentVersion(archive *comparch.ComponentArchive, repo cpi.Repository) (ocm.ComponentVersionAccess, error) PushComponentVersion(archive *comparch.ComponentArchive, repository cpi.Repository, overwrite bool) error + DescriptorResourcesAreEquivalent(archive *comparch.ComponentArchive, remoteVersion ocm.ComponentVersionAccess) bool } type OciRepo struct{} @@ -45,3 +49,49 @@ func (r *OciRepo) PushComponentVersion(archive *comparch.ComponentArchive, repo } return nil } + +func (r *OciRepo) DescriptorResourcesAreEquivalent(archive *comparch.ComponentArchive, + remoteVersion ocm.ComponentVersionAccess) bool { + localResources := archive.GetDescriptor().Resources + remoteResources := remoteVersion.GetDescriptor().Resources + if len(localResources) != len(remoteResources) { + return false + } + + localResourcesMap := map[string]compdesc.Resource{} + for _, res := range localResources { + localResourcesMap[res.Name] = res + } + + for _, res := range remoteResources { + localResource := localResourcesMap[res.Name] + if res.Name == RawManifestLayerName { + remoteAccess, ok := res.Access.(*runtime.UnstructuredVersionedTypedObject) + if !ok { + return false + } + + _, ok = localResourcesMap[res.Name] + if !ok { + return false + } + localAccessObject, ok := localResource.Access.(*localblob.AccessSpec) + if !ok { + return false + } + + remoteAccessLocalReference, ok := remoteAccess.Object[accessLocalReferenceFieldName].(string) + if !ok { + return false + } + // Trimming 7 characters because locally the sha256 is followed by '.' but remote it is followed by ':' + if remoteAccessLocalReference[7:] != localAccessObject.LocalReference[7:] { + return false + } + } else if !res.IsEquivalent(&localResource) { + return false + } + } + + return true +} diff --git a/pkg/module/remote.go b/pkg/module/remote.go index 931edc2b9..b31fba6e7 100644 --- a/pkg/module/remote.go +++ b/pkg/module/remote.go @@ -11,7 +11,6 @@ import ( "github.com/open-component-model/ocm/pkg/contexts/credentials/repositories/dockerconfig" oci "github.com/open-component-model/ocm/pkg/contexts/oci/repositories/ocireg" "github.com/open-component-model/ocm/pkg/contexts/ocm" - "github.com/open-component-model/ocm/pkg/contexts/ocm/accessmethods/localblob" "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" "github.com/open-component-model/ocm/pkg/contexts/ocm/cpi" "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" @@ -143,8 +142,7 @@ func (r *Remote) Push(archive *comparch.ComponentArchive, overwrite bool) (ocm.C return nil, false, fmt.Errorf("could not lookup component version: %w", err) } - if descriptorResourcesAreEquivalent(archive.GetDescriptor().Resources, - versionAccess.GetDescriptor().Resources) { + if r.DescriptorResourcesAreEquivalent(archive, versionAccess) { return versionAccess, false, nil } return nil, false, fmt.Errorf("version %s already exists with different content, please use "+ @@ -171,46 +169,3 @@ func (h *customTransferHandler) TransferVersion(repo ocm.Repository, src ocm.Com error) { return h.TransferHandler.TransferVersion(repo, src, meta, tgt) } - -func descriptorResourcesAreEquivalent(localResources, remoteResources compdesc.Resources) bool { - if len(localResources) != len(remoteResources) { - return false - } - - localResourcesMap := map[string]compdesc.Resource{} - for _, res := range localResources { - localResourcesMap[res.Name] = res - } - - for _, res := range remoteResources { - localResource := localResourcesMap[res.Name] - if res.Name == RawManifestLayerName { - remoteAccess, ok := res.Access.(*runtime.UnstructuredVersionedTypedObject) - if !ok { - return false - } - - _, ok = localResourcesMap[res.Name] - if !ok { - return false - } - localAccessObject, ok := localResource.Access.(*localblob.AccessSpec) - if !ok { - return false - } - - remoteAccessLocalReference, ok := remoteAccess.Object[accessLocalReferenceFieldName].(string) - if !ok { - return false - } - // Trimming 7 characters because locally the sha256 is followed by '.' but remote it is followed by ':' - if remoteAccessLocalReference[7:] != localAccessObject.LocalReference[7:] { - return false - } - } else if !res.IsEquivalent(&localResource) { - return false - } - } - - return true -} diff --git a/pkg/module/remote_test.go b/pkg/module/remote_test.go index 2023e6728..e5be9413a 100644 --- a/pkg/module/remote_test.go +++ b/pkg/module/remote_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/mandelsoft/vfs/pkg/memoryfs" - "github.com/open-component-model/ocm/pkg/contexts/ocm" "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" ocmv1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1" "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" @@ -60,25 +59,18 @@ func TestRemote_Push(t *testing.T) { compdesc.DefaultResources(cd) archive, _ := module.CreateArchive(archiveFS, "./mod", cd) - ociRepoAccessMock := mocks.OciRepoAccess{} - ociRepoAccessMock.On("PushComponentVersion", archive, mock.Anything, mock.Anything).Return(nil) - ociRepoAccessMock.On("GetComponentVersion", archive, - mock.Anything).Return(mock.AnythingOfType("internal.ComponentVersionAccess")) - - type fields struct { - OciRepoAccess module.OciRepoAccess - } + var ociRepoAccessMock mocks.OciRepoAccess type args struct { archive *comparch.ComponentArchive overwrite bool } tests := []struct { - name string - fields fields - args args - want ocm.ComponentVersionAccess - want1 bool - wantErr assert.ErrorAssertionFunc + name string + versionAlreadyExists bool + sameContent bool + args args + wantIsPushed bool + assertFn func(err error, i ...interface{}) }{ { name: "Same version, same content, with overwrite flag", @@ -86,38 +78,121 @@ func TestRemote_Push(t *testing.T) { overwrite: true, archive: archive, }, - fields: fields{ - OciRepoAccess: &ociRepoAccessMock, + assertFn: func(err error, i ...interface{}) { + assert.NoError(t, err) + ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 0) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 0) }, + wantIsPushed: true, + versionAlreadyExists: false, + sameContent: true, }, { name: "Same version, same content, without overwrite flag", + args: args{ + overwrite: false, + archive: archive, + }, + assertFn: func(err error, i ...interface{}) { + assert.NoError(t, err) + ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 0) + ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 1) + }, + wantIsPushed: false, + versionAlreadyExists: true, + sameContent: true, }, { name: "Same version, different content, with overwrite flag", + args: args{ + overwrite: true, + archive: archive, + }, + assertFn: func(err error, i ...interface{}) { + assert.NoError(t, err) + ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 0) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 0) + }, + wantIsPushed: true, + versionAlreadyExists: true, + sameContent: false, }, { name: "Same version, different content, without overwrite flag", + args: args{ + overwrite: false, + archive: archive, + }, + assertFn: func(err error, i ...interface{}) { + assert.Errorf(t, err, + "version 1.0.0 already exists with different content, please use --module-archive-version-overwrite flag to overwrite it") + ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 0) + ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 1) + }, + wantIsPushed: false, + versionAlreadyExists: true, + sameContent: false, }, { name: "different version, with overwrite flag", + args: args{ + overwrite: true, + archive: archive, + }, + assertFn: func(err error, i ...interface{}) { + assert.NoError(t, err) + ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 0) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 0) + }, + wantIsPushed: true, + versionAlreadyExists: false, + sameContent: false, }, { name: "different version, without overwrite flag", + args: args{ + overwrite: false, + archive: archive, + }, + assertFn: func(err error, i ...interface{}) { + assert.NoError(t, err) + ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 0) + }, + wantIsPushed: true, + versionAlreadyExists: false, + sameContent: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + ociRepoAccessMock = mocks.OciRepoAccess{} + ociRepoAccessMock.On("PushComponentVersion", archive, mock.Anything, mock.Anything).Return(nil) + ociRepoAccessMock.On("GetComponentVersion", archive, + mock.Anything).Return(nil, nil) + ociRepoAccessMock.On("ComponentVersionExists", archive, mock.Anything).Return(tt.versionAlreadyExists, nil) + ociRepoAccessMock.On("DescriptorResourcesAreEquivalent", mock.Anything, + mock.Anything).Return(tt.sameContent) r := &module.Remote{ Insecure: true, - OciRepoAccess: tt.fields.OciRepoAccess, - } - got, got1, err := r.Push(tt.args.archive, tt.args.overwrite) - if !tt.wantErr(t, err, fmt.Sprintf("Push(%v, %v)", tt.args.archive, tt.args.overwrite)) { - return + OciRepoAccess: &ociRepoAccessMock, } - assert.Equalf(t, tt.want, got, "Push(%v, %v)", tt.args.archive, tt.args.overwrite) - assert.Equalf(t, tt.want1, got1, "Push(%v, %v)", tt.args.archive, tt.args.overwrite) + _, isPushed, err := r.Push(tt.args.archive, tt.args.overwrite) + tt.assertFn(err, fmt.Sprintf("Push(%v, %v)", tt.args.archive, tt.args.overwrite)) + + assert.Equalf(t, tt.wantIsPushed, isPushed, "Push(%v, %v)", tt.args.archive, tt.args.overwrite) }) } } From cd62628a0563ada305f7c550eff9aa93c8438e0c Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Thu, 30 Nov 2023 12:27:37 +0100 Subject: [PATCH 9/9] Code review comments --- cmd/kyma/alpha/create/module/module.go | 25 ++++++++++++++++++++-- pkg/module/remote.go | 29 +++++++++++++------------- pkg/module/remote_test.go | 23 +++++++++----------- 3 files changed, 48 insertions(+), 29 deletions(-) diff --git a/cmd/kyma/alpha/create/module/module.go b/cmd/kyma/alpha/create/module/module.go index 5f4580232..3a8fbfaa7 100644 --- a/cmd/kyma/alpha/create/module/module.go +++ b/cmd/kyma/alpha/create/module/module.go @@ -13,8 +13,10 @@ import ( "github.com/mandelsoft/vfs/pkg/memoryfs" "github.com/mandelsoft/vfs/pkg/osfs" "github.com/mandelsoft/vfs/pkg/vfs" + "github.com/open-component-model/ocm/pkg/contexts/ocm" "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" compdescv2 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/versions/v2" + "github.com/open-component-model/ocm/pkg/contexts/ocm/cpi" "github.com/spf13/cobra" "go.uber.org/zap" "gopkg.in/yaml.v3" @@ -359,14 +361,33 @@ func (cmd *command) Run(ctx context.Context) error { cmd.CurrentStep.Failure() return err } - componentVersionAccess, isPushed, err := remote.Push(archive, cmd.opts.ArchiveVersionOverwrite) + + repo, err := remote.GetRepository(cpi.DefaultContext()) + if err != nil { + cmd.CurrentStep.Failure() + return err + } + + var componentVersionAccess ocm.ComponentVersionAccess + shouldPushArchive, err := remote.ShouldPushArchive(repo, archive, cmd.opts.ArchiveVersionOverwrite) if err != nil { cmd.CurrentStep.Failure() return err } - if isPushed { + + if shouldPushArchive { + componentVersionAccess, err = remote.Push(repo, archive, cmd.opts.ArchiveVersionOverwrite) + if err != nil { + cmd.CurrentStep.Failure() + return err + } cmd.CurrentStep.Successf("Module successfully pushed") } else { + componentVersionAccess, err = remote.GetComponentVersion(archive, repo) + if err != nil { + cmd.CurrentStep.Failure() + return err + } cmd.CurrentStep.Successf(fmt.Sprintf("Module already exists. Retrieved image from %q", cmd.opts.RegistryURL)) } diff --git a/pkg/module/remote.go b/pkg/module/remote.go index b31fba6e7..01e9642b1 100644 --- a/pkg/module/remote.go +++ b/pkg/module/remote.go @@ -125,39 +125,40 @@ func NoSchemeURL(url string) string { return regex.ReplaceAllString(url, "") } -// Push picks up the archive described in the config and pushes it to the provided registry if not existing. -// The credentials and token are optional parameters -func (r *Remote) Push(archive *comparch.ComponentArchive, overwrite bool) (ocm.ComponentVersionAccess, bool, error) { - repo, err := r.GetRepository(archive.GetContext()) - if err != nil { - return nil, false, err - } - +func (r *Remote) ShouldPushArchive(repo cpi.Repository, archive *comparch.ComponentArchive, overwrite bool) (bool, + error) { if !overwrite { versionExists, _ := r.ComponentVersionExists(archive, repo) if versionExists { versionAccess, err := r.GetComponentVersion(archive, repo) if err != nil { - return nil, false, fmt.Errorf("could not lookup component version: %w", err) + return false, fmt.Errorf("could not lookup component version: %w", err) } if r.DescriptorResourcesAreEquivalent(archive, versionAccess) { - return versionAccess, false, nil + return false, nil } - return nil, false, fmt.Errorf("version %s already exists with different content, please use "+ + return false, fmt.Errorf("version %s already exists with different content, please use "+ "--module-archive-version-overwrite flag to overwrite it", archive.ComponentVersionAccess.GetVersion()) } } - if err = r.PushComponentVersion(archive, repo, overwrite); err != nil { - return nil, false, err + return true, nil +} + +// Push picks up the archive described in the config and pushes it to the provided registry. +// The credentials and token are optional parameters +func (r *Remote) Push(repo cpi.Repository, archive *comparch.ComponentArchive, + overwrite bool) (ocm.ComponentVersionAccess, error) { + if err := r.PushComponentVersion(archive, repo, overwrite); err != nil { + return nil, err } componentVersion, err := r.GetComponentVersion(archive, repo) - return componentVersion, err == nil, err + return componentVersion, err } type customTransferHandler struct { diff --git a/pkg/module/remote_test.go b/pkg/module/remote_test.go index e5be9413a..78f30b466 100644 --- a/pkg/module/remote_test.go +++ b/pkg/module/remote_test.go @@ -7,6 +7,7 @@ import ( "github.com/mandelsoft/vfs/pkg/memoryfs" "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" ocmv1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1" + "github.com/open-component-model/ocm/pkg/contexts/ocm/cpi" "github.com/open-component-model/ocm/pkg/contexts/ocm/repositories/comparch" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -48,7 +49,7 @@ func TestNoSchemaURL(t *testing.T) { } } -func TestRemote_Push(t *testing.T) { +func TestRemote_ShouldPushArchive(t *testing.T) { archiveFS := memoryfs.New() cd := &compdesc.ComponentDescriptor{} cd.ComponentSpec.SetName("test.io/module/test") @@ -60,6 +61,7 @@ func TestRemote_Push(t *testing.T) { archive, _ := module.CreateArchive(archiveFS, "./mod", cd) var ociRepoAccessMock mocks.OciRepoAccess + type args struct { archive *comparch.ComponentArchive overwrite bool @@ -81,8 +83,7 @@ func TestRemote_Push(t *testing.T) { assertFn: func(err error, i ...interface{}) { assert.NoError(t, err) ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 0) - ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) - ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 0) ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 0) }, wantIsPushed: true, @@ -99,7 +100,6 @@ func TestRemote_Push(t *testing.T) { assert.NoError(t, err) ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 1) ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) - ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 0) ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 1) }, wantIsPushed: false, @@ -115,8 +115,7 @@ func TestRemote_Push(t *testing.T) { assertFn: func(err error, i ...interface{}) { assert.NoError(t, err) ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 0) - ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) - ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 0) ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 0) }, wantIsPushed: true, @@ -134,7 +133,6 @@ func TestRemote_Push(t *testing.T) { "version 1.0.0 already exists with different content, please use --module-archive-version-overwrite flag to overwrite it") ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 1) ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) - ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 0) ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 1) }, wantIsPushed: false, @@ -150,8 +148,7 @@ func TestRemote_Push(t *testing.T) { assertFn: func(err error, i ...interface{}) { assert.NoError(t, err) ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 0) - ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) - ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 0) ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 0) }, wantIsPushed: true, @@ -167,8 +164,7 @@ func TestRemote_Push(t *testing.T) { assertFn: func(err error, i ...interface{}) { assert.NoError(t, err) ociRepoAccessMock.AssertNumberOfCalls(t, "ComponentVersionExists", 1) - ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 1) - ociRepoAccessMock.AssertNumberOfCalls(t, "PushComponentVersion", 1) + ociRepoAccessMock.AssertNumberOfCalls(t, "GetComponentVersion", 0) ociRepoAccessMock.AssertNumberOfCalls(t, "DescriptorResourcesAreEquivalent", 0) }, wantIsPushed: true, @@ -189,10 +185,11 @@ func TestRemote_Push(t *testing.T) { Insecure: true, OciRepoAccess: &ociRepoAccessMock, } - _, isPushed, err := r.Push(tt.args.archive, tt.args.overwrite) + repo, _ := r.GetRepository(cpi.DefaultContext()) + got, err := r.ShouldPushArchive(repo, tt.args.archive, tt.args.overwrite) tt.assertFn(err, fmt.Sprintf("Push(%v, %v)", tt.args.archive, tt.args.overwrite)) - assert.Equalf(t, tt.wantIsPushed, isPushed, "Push(%v, %v)", tt.args.archive, tt.args.overwrite) + assert.Equalf(t, tt.wantIsPushed, got, "Push(%v, %v)", tt.args.archive, tt.args.overwrite) }) } }