feat: Read component descriptor from oci registry #2736

Tomasz-Smelcerz-SAP · 2025-09-15T07:55:40Z

Description

Note to reviewers: I was really trying to keep this PR as minimal as possible. There are many things to fix/improve/cleanup and many times I've seen that, but I decided not to, in order to introduce as few changes as possible.
Keep that in mind when suggesting improvements. Do we really want to make this PR even bigger? I would rather prefer to make a note in the sources and then create a follow-up PR to clean all the notes. Just remember that the work remaining in #2526 and our other efforts (like simplifying/getting rid of templatelookup.go) will probably make most of these review-fixes obsolete or unnecessary.
Up to you.

Changes proposed in this pull request:

Component Descriptors (CD) are now read from the OCM repository (which usually is some OCI registry)
To read a CD, one must provide an OCM Component Name and a version. In our current setup, the OCM Component Name is available only in a ModuleReleaseMeta for a given Module.
It means that with this PR the ModuleReleaseMeta becomes mandatory
To keep this PR minimal, I did not remove code/tests related to the previous "modes" of operation, where CD was fetched from a ModuleTemplate and ModuleReleaseMeta was, in some cases, optional. This code requires cleanup in a separate PR. Of course when the test is failing, I had to refactor it
An interesting case: What to do if user configures a Module for which there is no ModuleReleaseMeta? Currently, when user configures a module on a Kyma, for which there's no ModuleTemplate, the Kyma is put in Warning state. To keep it consistent, I use the same error->status resolution strategy. To be discussed.
Code coverage for some packages (templatelookup) decreased. This is because some test cases were disabled. This is (mostly) a legacy code that will disappear anyway. A new method of resolving ModuleTemplates along with ModuleReleaseMeta should be introduced, but this is not a part of this PR. I think it is OK to temporarily "lower the bar" - we're in the middle of a bigger refactoring here. Related issues that will introduce the necessary cleanpup/changes are listed are listed here
There is an open discussion that focuses on the little detail: The OCI registry URL. Previously it was taken from the static ComponentDescriptor data embedded in the ModuleTemplate. Now we have at least two different values we can use: An explicitly configured one and the one from ComponentDescriptor, after it is successfully fetched (we still need OCI Registry URL for subsequent operations, like raw-manifest fetching). Which URL to choose? Please take a look at the discussion. I decided to rely on explicitly provided value as I had no feedback that I should chose a different strategy here.

Related issue(s)
#2601

github-actions · 2025-09-16T11:45:47Z

❌ Manifests created with 'make dry-run-control-plane' changed! Please make sure to check if changes are needed in related repositories like management-plane-charts, runtime-watcher, etc.

Tomasz-Smelcerz-SAP · 2025-09-23T07:53:48Z

We are blocked here by: <internal-repo>/kyma/test-infra/issues/781 - we need all the ModuleReleaseMeta to have OCM version related attributes. In particular spec.mandatory.version

github-actions · 2025-10-01T09:35:03Z

⚠️ Pipeline-related file changes detected! Please review if related updates (e.g. manifest generation or workflow adjustments) are required.

pkg/testutils/moduletemplate.go

github-actions · 2025-10-07T18:54:30Z

❌ Manifests created with 'make dry-run-control-plane' changed! Please make sure to check if changes are needed in related repositories like management-plane-charts, runtime-watcher, etc.

Tomasz-Smelcerz-SAP · 2025-10-14T05:50:09Z

pkg/testutils/moduletemplate.go

 }

-func GetModuleTemplate(ctx context.Context,
+func GetModuleTemplateInfo(ctx context.Context,


Review Note: I changed the return type of this function for a reason.
Previously the returned ModuleTemplate instance was enough to also get the OCM descriptor, as it was embedded in the ModuleTemplate.
For an example, look how the "ReadModuleVersionFromModuleTemplate" function (now renamed to "GetOCMVersionForModule" was making use of this data (before current changes).

Now, in order to find an OCM descriptor, or at least an OCM identitfier for the given ModuleTemplate, we cannot rely on the ModuleTemplate instance alone.
But there are some callers of this function that still need this information, for example: the mentioned "ReadModuleVersionFromModuleTemplate" function.

What can we do to support these callers?
Please note that although ModuleTemplate does not provide ComponentDescriptor anymore (or rather we don't want to use it anymore), the ModuleTemplateInfo object contains the data we need: the ModuleTemplateInfo implements the OCMIdentity interface.
And the good news is: we have a ready to use ModuleTemplateInfo instance here.
Thus it's a natural choice to make use of this instance (as we have it anyway).
The change to multiple return values in signature is intentional: it forces the compiler to return errors in all places where this function was used - and because of that, ensures that I fixed the code in all of these places.

internal/repository/modulereleasemeta/repository.go

c-pius

Intermediate review. Have not looked at test files and other chore like .github yet.

api/v1beta2/modulereleasemeta_types.go

internal/controller/mandatorymodule/deletion_controller.go

c-pius · 2025-10-15T06:17:20Z

internal/controller/mandatorymodule/installation_controller.go

+			continue
+		}
+
+		if template.ComponentIdentity == nil {


What is the scenario where this is not nil? As far as I can see, the only other place where we write this is the "by module release meta lookup strategy" which is not used for mandatory modules.

c-pius · 2025-10-15T06:30:34Z

internal/descriptor/provider/provider.go

+// Convenience interface to get the OCM identity of a component from objects
+// that already have all required data.
+// Then we don't have to create intermediate variables of type ocmidentity.Component.
+type OCMIProvider interface {
+	GetOCMIdentity() (*ocmidentity.Component, error)
+}


Wondering if the ModuleTemplateInfo could also directly implement the ocmidentity.Component interface (Name(), Version()). Then we wouldn't need this interface and could directly pass the ModuleTemplateInfo. But the function names are rather generic, so also not too sure if this is a good idea. But since we own the ocmidentity.Component interface, we could also make the methods more specific (GetOcmComponentName(), GetOcmComponentVersion()).

But if we keep the interface, could we move the type up to before the constructor? I think it is easier to read if the file is ordered like "types -> vars -> constructor -> funcs"

c-pius · 2025-10-15T06:43:54Z

internal/descriptor/provider/provider.go

-		if desc == nil {
-			return nil, ErrDescriptorNil
-		}
+func (c *CachedDescriptorProvider) Add(ocmi ocmidentity.Component) error {


Good catch actually. I am a bit confused about the usage and exact implementation of the provider though. In regular.go.

For the same module (ocmi), we always first add (updateCache: true), and then get (updateCache:false). Why not only doing a get that is updating the cache in the beginning and get rid of Add? Also, I think the updateCache bool is also misleading. I would expect that this forces the cache to update. But instead, it is only controlling if the cache is updated AFTER not finding the descriptor in the cache. Not sure what the purpose is for that?

for _, moduleInfo := range FetchModuleInfo(kyma) { // ... if err := t.descriptorProvider.Add(*ocmi); err != nil { templateInfo.Err = fmt.Errorf("failed to get descriptor: %w", err) templates[moduleInfo.Name] = &templateInfo continue } for i := range kyma.Status.Modules { moduleStatus := &kyma.Status.Modules[i] if moduleMatch(moduleStatus, moduleInfo.Name) { descriptor, err := t.descriptorProvider.GetDescriptor(*ocmi) if err != nil { msg := "could not handle channel skew as descriptor from template cannot be fetched" templateInfo.Err = fmt.Errorf("%w: %s", ErrTemplateUpdateNotAllowed, msg) continue } markInvalidSkewUpdate(ctx, &templateInfo, moduleStatus, descriptor.Version) } }

c-pius · 2025-10-15T09:01:47Z

internal/service/componentdescriptor/service.go

+type OCIRepository interface {
+	GetConfigFile(ctx context.Context, name, tag string) ([]byte, error)
+	PullLayer(ctx context.Context, name, tag, digest string) (containerregistryv1.Layer, error)
+}


How about passing the ocmidentity.Component directly instead of separated name and tag (version)

Digest could also use the digest type. In the call below, the typecast should not be needed anyway.

How about passing the ocmidentity.Component directly instead of separated name and tag (version)

I was thinking about it, but it is "polluting" OCIRepository with an abstraction that doesn't belong there (OCM is not something that OCI Repository "knows" about). We could have a dedicated type that represents <name, tag, digest> and do the conversion. Because this repository is very simple, I decided for just strings. But I am always in favor of having a dedicated type.
Should I introduce it?
Or we could use the fact that the Component type offers methods: Name() and Version() - we could accept an interface that matches these methods.

c-pius · 2025-10-15T09:09:32Z

internal/service/componentdescriptor/service.go

+// SimplifiedLayer represents only necessary part of containerregistryv1.Layer to simplify testing.
+type SimplifiedLayer interface {
+	Digest() (containerregistryv1.Hash, error)
+	Uncompressed() (io.ReadCloser, error)
+}


This should not be needed. We can have func extractFile(ioh extractFileIOHelper, layer containerregistryv1.Layer, fileName string) ([]byte, error) { with mock layer

type mockLayer struct { containerregistryv1.Layer errOnUncompressed error errOnDigest error }

Needed Uncompressed() and Digest() are overriden. Rest are implicitly exposed but will not work since we didn't provide an actual Layer

Thanks for catching that: Indeed SimplifiedLayer shouldn't be exported, it should be a package internal type - then we don't pollute our API with internals.
After that change I'd say it's a matter of taste. I prefer explicit typing (function is given only the the abstraction it absolutely needs), even if it means introducing a new interface. If it is unexported, nobody ~~knows~~ cares, and performance-wise it is exactly the same I think?
I somehow don't like the fact that mockLayer will now panic: runtime error: invalid memory address or nil pointer dereference when invoked with a non-implemented method, even though we're not doing that now - but who knows the future?
But maybe it's just my code-related OCD :)
I will change that as you suggested, but If we'll ever see panic here, then I can say: ‘See? I told you!’ :)

c-pius · 2025-10-15T09:11:25Z

internal/service/componentdescriptor/service.go

+type extractFileIOHelper interface {
+	ReadAll(r io.Reader) ([]byte, error) // Reads all data from the reader
+}
+
+type defaultExtractFileIOHelper struct {
+	readAllFunc func(r io.Reader) ([]byte, error)
+}
+
+func (d *defaultExtractFileIOHelper) ReadAll(r io.Reader) ([]byte, error) {
+	return d.readAllFunc(r)
+}


same comment as for the wrapper in ocirepo.go. If we don't decide to go that we, I would at least name it consistently with Wrapper or Helper because eventually it is the same thing.

c-pius · 2025-10-15T09:11:36Z

internal/service/componentdescriptor/service.go

+// untarIoHelper abstracts the methods of tar.Reader and io package used in unTar function, for better testability.
+type untarIoHelper interface {
+	Next() (*tar.Header, error)                              // tar.Reader.Next()
+	CopyN(dst io.Writer, n int64) (written int64, err error) // modified io.CopyN()
+}
+
+type defaultUntarIOHelper struct {
+	tarReader *tar.Reader
+}
+
+func (d *defaultUntarIOHelper) Next() (*tar.Header, error) {
+	return d.tarReader.Next() //nolint:wrapcheck // this helper should be transparent
+}
+
+func (d *defaultUntarIOHelper) CopyN(dst io.Writer, n int64) (int64, error) {
+	return io.CopyN(dst, d.tarReader, n) //nolint:wrapcheck // this helper should be transparent
+}


same comment as for the wrapper in ocirepo.go. If we don't decide to go that we, I would at least name it consistently with Wrapper or Helper because eventually it is the same thing.

c-pius · 2025-10-15T09:23:37Z

pkg/templatelookup/regular.go

+// Implements provider.OCMIProvider interface.
+func (m ModuleTemplateInfo) GetOCMIdentity() (*ocmidentity.Component, error) {
+	if m.ComponentIdentity == nil {
+		return nil, fmt.Errorf("%w for module template %s", ErrNoIdentity, m.Name)
+	}
+	return m.ComponentIdentity, nil
 }


See comment in provider.go

ruanxin · 2025-10-15T13:37:31Z

cmd/main.go

+	ociRegistryHost := getOciRegistryHost(mgr.GetConfig(), flagVar, logger)
+	var insecure bool
+
+	if noSchemeRef, found := strings.CutPrefix(ociRegistryHost, "http://"); found {
+		insecure = true
+		ociRegistryHost = noSchemeRef
+	} else if noSchemeRef, found := strings.CutPrefix(ociRegistryHost, "https://"); found {
+		ociRegistryHost = noSchemeRef
+	}


this whole block can be getOciRegistryHost, and I would design this ociRegistryHost as url.URL type, you can use url.Parse(rawURL string), then you don't need to deal with cutting prefix, validating anymore. And you can use Scheme field to determine if it's insecure, so there is no need to claim insecure here and pass to NewRepository

I know that, but this is the existing code - and it returns a string, not a url.URL...
Of course I could refactor that and tens of other places in the code. But read this, I've just added this disclaimer as the first note in the PR description:

Note to reviewers: I was really trying to keep this PR as minimal as possible. There are many things to fix/improve/cleanup and many times I've seen that, but I decided not to, in order to introduce as few changes as possible. Keep that in mind when suggesting improvements. Do we really want to make this PR even bigger? I would rather prefer to make a note in the sources and then create a follow-up PR to clean all the notes. Just remember that the work remaining in https://github.com/kyma-project/lifecycle-manager/issues/2526 and our other efforts (like simplifying/getting rid of templatelookup.go) will probably make most of these review-fixes obsolete or unnecessary. Up to you.

That’s totally fine — the review comments are meant to highlight potential issues, not necessarily to be fixed right away. If you’d prefer not to address them in this PR, feel free to create a follow-up issue instead.

At the same time, not mentioning these code smells would be my oversight. Continuously improving our existing codebase should always be our goal.

ruanxin · 2025-10-15T13:55:31Z

internal/repository/oci/ocirepo.go

+	keyChainLookup spec.KeyChainLookup
+	hostref        string
+	insecure       bool
+	cWrapper       craneWrapper // in runtime delegates to crane package functions


I will rename this interface more generic to reflect it's usage, it's basically to manage container images, right? why not something like ContainerImageManager? crane is just a specific implementation.

The answer is: It uses types from crane library, is strictly tied to it. It is not generic. I can rename it, but these types will still point out to the crane library. It will be misleading then: The type name will suggest a generic abstraction, but the interface methods will force you to have just a single, crane-based implementation.

I disagree with your answer. An interface should always be designed to be generic, regardless of the number of existing implementations. The point of having an interface is not to describe the current dependency (in this case, crane), but to define a stable abstraction boundary between your business logic and the external library.

The consumer of this interface shouldn’t need to know or care what crane is; it should only rely on the capability it provides, in this case, it's "pulling a container image." However, what you are trying to do now is make the interface even deeply coupled with the lower-level details, which is certainly against the Dependency Inversion Principle (DIP): High-level modules shouldn’t depend on low-level details (like the crane library). Both should depend on abstractions.

ruanxin · 2025-10-15T13:59:15Z

internal/repository/oci/ocirepo.go

+		keyChainLookup: kcl,
+		hostref:        hostref,
+		insecure:       insecure,
+		cWrapper:       &defaultCraneWrapper{},


We are agreed in ADR to introduce dependencies via Constructor Injection, but not using this implecity way, it hardcoded with &defaultCraneWrapper{}

I don't agree.

In the ADR there's a link to a poorly written article, that states:
[Dependency Inversion] is a principle whose name is often misused interchangeable with Dependency Injection even it is not the same.
and:

The principle consists of two core concepts: high-level modules should not depend on low-level modules, and both should depend on abstractions.

First: We don't have any "modules" (plural) here - there's just one "module". The craneWrapper is an unexported, (package-private) interface created to facilitate testing. The purpose? To be able to use "real" crane library in production, and a mock implementation for testing.
Yes, we are depending on the crane library, but no: we don't want to have user-injected "concrete implementations" here.
Contrary to the examples given in the mentioned article, this abstraction is not external, like a ConcreteUserRepository, or SqlClient, or something very common in our code: client.Client. I agree that such abstractions, external to the package, should be managed by Dependency Injection, either by using Constructor functions or in some other way. We agreed on using Constructors, fine.

But this abstraction is totally internal to the package.
None of the arguments given in the article applies:

Changing the dependency is not a concern here. We'll rather have a different repository implementation if we want to use something else than the crane library. And it will be defined in a different package.

Testing is not a concern: In Go (unlike Java in the article) we can test this without any problems as you can see. This is officially supported and promoted way of testing things in Go.

That's why I think we shouldn't expose craneWrapper as a constructor parameter: This would be an abstraction leak: We'd be leaking internal details of which library is used to communicate with the OCI Registry.
In addition:

Users of the package should not know about craneWrapper - because they don't need this knowledge to use the API.

Users of the package should not be forced to configure the constructor with a craneWrapper argument even though it doesn't bring any value to them. This implementation is fixed, there are no other "concrete implementations" to choose from. Formal parameters exists in programming languages to give users the freedom of providing different actual arguments. There is no such choice in that case.

In summary, exposing this in the constructor violates the following rules/practices:

Information Hiding (we're exposing an internal testing-related facility to the world)

Law of Demeter (Principle of Least Knowledge)

Encapsulation of the package

And again, we're not violating the "Dependency Inversion" because we don't have different modules (packages) here.

ruanxin · 2025-10-15T14:10:55Z

internal/repository/oci/ocirepo.go

+// RepositoryReader provides basic support to read data from OCI repositories.
+type RepositoryReader struct {
+	keyChainLookup spec.KeyChainLookup
+	hostref        string


Each part of URL have a standard name, this is just host, I never heard of hostref.

ruanxin · 2025-10-15T14:30:04Z

internal/repository/oci/ocirepo.go

+		return nil, fmt.Errorf("%w: %q", ErrNoProtocolScheme, hostref)
+	}
+
+	if strings.HasPrefix(hostref, "/") {


Please check my comment here https://github.com/kyma-project/lifecycle-manager/pull/2736/files#r2432603480, if you already pass hostref as url.URL, each section of the url address belongs to a specific field (Scheme, Host, Path ...), you don't need to validate host anymore.

ruanxin · 2025-10-15T14:31:14Z

internal/repository/oci/ocirepo.go

+}
+
+func (s *RepositoryReader) toImageRef(name, tag string) string {
+	hostPath := path.Join(s.hostref, "component-descriptors", name)


We have const value defined DefaultRepoSubdirectory

ruanxin · 2025-10-15T14:43:08Z

internal/controller/mandatorymodule/installation_controller.go

+	// Note: Here we're just adding OCM identity information.
+	// It doesn't change how the Mandatory Modules are selected for installation:
+	// we still take the latest version of every ModuleTemplate which is marked as mandatory.
+	// The switch to the logic based on ModuleReleaseMeta will be done in a follow-up PR.


Please reference the related issue regarding switch to the logic based on ModuleReleaseMeta here. will be done in a follow-up PR doesn't bring documentation value.

ruanxin · 2025-10-15T14:53:00Z

internal/descriptor/provider/provider.go

+func NewCachedDescriptorProvider(service DescriptorService) *CachedDescriptorProvider {
 	return &CachedDescriptorProvider{
-		DescriptorCache: cache.NewDescriptorCache(),
+		descriptorCache:   cache.NewDescriptorCache(),


This descriptorCache should also be injected via constructor based on our ADR

ruanxin · 2025-10-15T14:58:45Z

internal/descriptor/provider/provider.go

-func (c *CachedDescriptorProvider) Add(template *v1beta2.ModuleTemplate) error {
-	if template == nil {
-		return ErrTemplateNil
+func (c *CachedDescriptorProvider) getDescriptor(ocmi ocmidentity.Component, updateCache bool) (


I don't think it's good approach mix the update cache together with get, they are seperate logic. And you only need update cache in Add, there is no need introduce such flag, also vailate single responsiblity.

OK, this is an internal function... The code used to be in two functions: Add and Get, but I realized the code is almost identical. So I thought the reviewers will point out, that the code can be shared...
But OK, let's duplicate it again. no problem for me.

internal/descriptor/provider/provider.go

ruanxin · 2025-10-15T15:06:01Z

internal/service/componentdescriptor/fakeservice.go

+// where a single descriptor is used for multiple test cases with slightly different module names.
+// Defining this fake here has the advantage of using the same internal
+// deserialization logic as the real service, which makes this fake a bit more "real".
+type FakeService struct {


This one should exist under _test package specifically.

ruanxin · 2025-10-15T15:08:19Z

internal/service/componentdescriptor/untar_internal_test.go

@@ -0,0 +1,123 @@
+package componentdescriptor


rename to componentdescriptor_test

ruanxin · 2025-10-15T15:08:31Z

internal/service/componentdescriptor/extractfile_internal_test.go

@@ -0,0 +1,74 @@
+package componentdescriptor


rename to componentdescriptor_test, and refactor those internal function you are testing as suggestion given here: https://github.com/kyma-project/lifecycle-manager/pull/2736/files#r2432968414

ruanxin · 2025-10-15T15:12:53Z

internal/service/componentdescriptor/untar_internal_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestUnTar(t *testing.T) {


This internal function is worth extracting into a dedicated struct member function, and inject into componentdescriptor Service. Once extracted, you can make it public and test it through its public method instead of testing this internal function directly.

Why would I do that? Nobody else (no package API user) wants to use that function and making it public increases package API surface for no reason.
I could extract this to a separate package instead, but then: What's the difference between that and just making the function public? I am still exposing a "private" thing via public API (and to make things worse I introduce a new package for that), although nobody asks for it.
That's a bad practice, package APIs should be minimal and hiding implementation details is exactly what packages are for.
BTW: Function doesn't have to be exported to be testable in Go. If in doubt, please see this official go docs
If the test file is in the same package, it may refer to unexported identifiers within the package, as in this example[...]
This is supported and promoted way of writing tests in Go.

pkg/templatelookup/regular.go

ruanxin · 2025-10-15T15:24:08Z

pkg/templatelookup/regular_test.go

+					*builder.NewModuleTemplateBuilder().
+						WithName(fmt.Sprintf("%s-%s", module.Name, testModule.Version)).
+						WithModuleName(module.Name).
+						WithChannel(module.Channel).


Since you already refactor this part, please do not assign channel to ModuleTemplate anymore, channel is in MRM now.

I did not touch "WithChannel" on purpose, because it's a journey down the rabbit hole of refactoring. Removing that means removing methods, errors, etc., which ultimately leads to doubling the work others are doing/planning in PR related to ModuleTemplate resolution strategies cleanup (by_channel_strategy). I prefer not to touch it and leave this cleanup to the PR that is addressing this specific concern.

lindnerby · 2025-10-16T06:54:17Z

internal/descriptor/types/ocmidentity/ocmidentity.go

+
+// Component uniquely identifies an OCM Component.
+// See: https://ocm.software/docs/overview/important-terms/#component-identity
+type Component struct {


ComponentId would be even more fitting as the structs name to reflect what it is.

Usade and function during the code even call this differently already:
OCMIdentity
Would be good to align this and settle for a name, to talk about one thing from the beginning.

lindnerby · 2025-10-16T06:55:46Z

internal/descriptor/types/ocmidentity/ocmidentity.go

+}
+
+// MustNew is a convenience constructor that panics if name or version are not provided.
+func MustNew(name, version string) *Component {


Probably this is not needed. It's even dangerous because someone might use it in production code.
Looks like this is anyways only needed in tests so any convenience function can live in test_utils.

lindnerby · 2025-10-16T06:57:44Z

internal/descriptor/types/ocmidentity/ocmidentity.go

+	}, nil
+}
+
+func (c *Component) Name() string {


Since these are only read methods (similar to getters) they can even have a value receiver instead of pointer to further indicate that:
func (c Component) Name() string {

lindnerby · 2025-10-16T07:07:22Z

internal/descriptor/cache/cache.go

+
+type DescriptorKey string
+
+func GenerateDescriptorKey(ocmi ocmidentity.Component) DescriptorKey {


Did not find a better or unused place to put this comment 😄
Once settled for a consistent name for ocmidentity.Component you should also adapt the the parameter names throughout the whole PR. So either: ocmId or componentId or ocmCompId or ocmIdentity, whatever the agreed name of the domain entity will be then.

lindnerby · 2025-10-16T07:17:24Z

internal/service/componentdescriptor/service.go

+		return nil, fmt.Errorf("%w for %s", ErrLayerNil, commonErrMsg(ocmi))
+	}
+
+	compDescLayerDigest := ocmArtifactConfig.ComponentDescriptorLayer.Digest


Until here all that is done is needed to get the digest. You can wrap everything in a private method getDescriptorLayerDigest(*) to indicate that. Also allows for a comment on the func to describe details.

lindnerby · 2025-10-16T07:23:22Z

internal/service/componentdescriptor/service.go

+	ReadAll(r io.Reader) ([]byte, error) // Reads all data from the reader
+}
+
+type defaultExtractFileIOHelper struct {


Can you explain what is the reasoning behind these internal interface and immediate struct implementations?
Can you think about using dependency injection for these helper functionalities?

As I see it the untarIoHelper as well as the defaultExtractFileIOHelper can be injected dependencies?

lindnerby · 2025-10-16T07:39:58Z

.github/actions/deploy-template-operator-with-modulereleasemeta/action.yml

+    - name: Create and apply ModuleReleaseMeta from the template-operator repo
      working-directory: template-operator
      if: ${{ matrix.e2e-test == 'kyma-metrics' ||
-        matrix.e2e-test == 'non-blocking-deletion' ||


Quick info on why this testcase is not important anymore? I don't know the details.

lindnerby · 2025-10-16T07:54:35Z

internal/descriptor/cache/key_test.go

+			ocmi := ocmidentity.MustNew(tc.moduleName, tc.moduleVersion)
+			got := cache.GenerateDescriptorKey(*ocmi)
+			assert.Equal(t, tc.want, string(got))
+		})


Just to align some things here:

DescriptorKey could also get its own file if the test gets it? Or incorporate this test as well in cache_test.go.

If there is only one testcase there is no need for a table driven structure. So either we come up with more testcases here or have just one plain test.

lindnerby · 2025-10-16T08:03:12Z

internal/repository/oci/export_test.go

@@ -0,0 +1,6 @@
+package oci
+
+// compiled only when running tests.


Why introduce this indirection and not create a separate pkg for the wrapper isolated with its tests.
And then inject the wrapper dependency for consumers as usual.

Why introduce another package "B" just to test package "A"?
Packages should have a purpose. In this case, package "A" - internal/repository/oci can "handle itself" very well, we can write all the tests we need without introducing new public abstractions.
I guess "do not multiply entities beyond necessity" is still the correct approach?

github-actions bot added the manifests-diff label Sep 16, 2025

Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch from f5357db to 6fca147 Compare September 24, 2025 05:38

Tomasz-Smelcerz-SAP marked this pull request as ready for review September 30, 2025 06:31

Tomasz-Smelcerz-SAP requested a review from a team as a code owner September 30, 2025 06:31

Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch from e258230 to 9d9eb17 Compare September 30, 2025 07:21

lindnerby linked an issue Sep 30, 2025 that may be closed by this pull request

Read component descriptor from oci registry #2601

Open

1 task

Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch 3 times, most recently from 0be33b5 to 3be024d Compare September 30, 2025 13:29

Tomasz-Smelcerz-SAP changed the title ~~Feat/read component descriptor from oci registry~~ feat: Read component descriptor from oci registry Sep 30, 2025

github-actions bot removed the manifests-diff label Sep 30, 2025

Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch 3 times, most recently from 071775d to 6aee460 Compare October 1, 2025 08:47

github-actions bot added the pipeline-changed label Oct 1, 2025

ruanxin added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2025

ruanxin reviewed Oct 1, 2025

View reviewed changes

pkg/testutils/moduletemplate.go Outdated Show resolved Hide resolved

Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch 3 times, most recently from d3509af to 5ac7c15 Compare October 7, 2025 18:52

github-actions bot added the manifests-diff label Oct 7, 2025

Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch from 1a7339b to 4552c9a Compare October 7, 2025 20:12

github-actions bot removed the manifests-diff label Oct 7, 2025

Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch 3 times, most recently from 916d93f to e3b1e83 Compare October 8, 2025 23:49

Tomasz-Smelcerz-SAP commented Oct 14, 2025

View reviewed changes

Tomasz-Smelcerz-SAP removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2025

LeelaChacha reviewed Oct 14, 2025

View reviewed changes

internal/repository/modulereleasemeta/repository.go Outdated Show resolved Hide resolved

c-pius reviewed Oct 15, 2025

View reviewed changes

ruanxin reviewed Oct 15, 2025

View reviewed changes

Tomasz-Smelcerz-SAP added 2 commits October 15, 2025 16:41

Read ComponentDescriptor from OCI registry

c7ddfe0

Cleanup

99ce9b3

ruanxin reviewed Oct 15, 2025

View reviewed changes

Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch from 6c9898e to 99ce9b3 Compare October 15, 2025 14:43

ruanxin reviewed Oct 15, 2025

View reviewed changes

internal/descriptor/provider/provider.go Outdated Show resolved Hide resolved

ruanxin reviewed Oct 15, 2025

View reviewed changes

pkg/templatelookup/regular.go Outdated Show resolved Hide resolved

ruanxin reviewed Oct 15, 2025

View reviewed changes

Tomasz-Smelcerz-SAP added 6 commits October 15, 2025 19:06

Review fix

ce5670b

Cleanup

b546aeb

Review fix

34eca26

Review fix

7341633

Review fix

b18d7aa

Review fix

08635e9

lindnerby requested changes Oct 16, 2025

View reviewed changes


		type DescriptorKey string

		func GenerateDescriptorKey(ocmi ocmidentity.Component) DescriptorKey {

		@@ -0,0 +1,6 @@
		package oci

		// compiled only when running tests.

feat: Read component descriptor from oci registry #2736

Are you sure you want to change the base?

feat: Read component descriptor from oci registry #2736

Conversation

Tomasz-Smelcerz-SAP commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 16, 2025

Uh oh!

Tomasz-Smelcerz-SAP commented Sep 23, 2025

Uh oh!

github-actions bot commented Oct 1, 2025

Uh oh!

Uh oh!

github-actions bot commented Oct 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

c-pius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tomasz-Smelcerz-SAP Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tomasz-Smelcerz-SAP Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruanxin Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tomasz-Smelcerz-SAP Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruanxin Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tomasz-Smelcerz-SAP Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruanxin Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tomasz-Smelcerz-SAP commented Sep 15, 2025 •

edited

Loading

Tomasz-Smelcerz-SAP Oct 15, 2025 •

edited

Loading

Tomasz-Smelcerz-SAP Oct 15, 2025 •

edited

Loading

ruanxin Oct 15, 2025 •

edited

Loading

Tomasz-Smelcerz-SAP Oct 15, 2025 •

edited

Loading

ruanxin Oct 15, 2025 •

edited

Loading

Tomasz-Smelcerz-SAP Oct 15, 2025 •

edited

Loading

ruanxin Oct 15, 2025 •

edited

Loading

Tomasz-Smelcerz-SAP Oct 16, 2025 •

edited

Loading

ruanxin Oct 15, 2025 •

edited

Loading

ruanxin Oct 15, 2025 •

edited

Loading

Tomasz-Smelcerz-SAP Oct 15, 2025 •

edited

Loading

Tomasz-Smelcerz-SAP Oct 15, 2025 •

edited

Loading