no-jira: v2/manifest/oci-manifest: refactor and dedup the code #1098

r4f4 · 2025-02-26T18:25:49Z

Description

Introduce a generic json/yaml parser functions
Refactor and dedup code in oci-manifest.go
Fixed unit tests to match error and/or return value
Add more unit tests

Github / Jira issue:

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Code Improvements (Refactoring, Performance, CI upgrades, etc)
Internal repo assets (diagrams / docs on github repo)
This change requires a documentation update on openshift docs

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Expected Outcome

Please describe the outcome expected from the tests.

sherine-k

thanks @r4f4
This helps a lot!
Few nits , but nothing critical.
Finally, sharing with you one last refactoring task we had in mind:
Move GetOperatorConfig and GetReleaseSchema out of this package and into operator and release packages instead.
In fact, these are used only by these domains. But they force us to mock these methods in almost all the unit tests where manifest package is needed.

sherine-k · 2025-02-27T10:34:00Z

v2/internal/pkg/common/parse.go

@@ -0,0 +1,63 @@
+//nolint:ireturn // generic T should be fine to return here
+package common


Great idea adding these methods

v2/internal/pkg/release/core-cincinnati.go

v2/internal/pkg/release/local_stored_collector.go

v2/internal/pkg/manifest/oci-manifest.go

sherine-k · 2025-02-27T11:04:17Z

v2/internal/pkg/manifest/oci-manifest.go

-func setInternalLog(log clog.PluggableLoggerInterface) {
-	if internalLog == nil {
-		internalLog = log
-	}


sherine-k · 2025-02-27T11:08:17Z

v2/internal/pkg/manifest/oci-manifest_test.go

-		log.Debug("completed test  %v ", res)
+		res, err := manifest.GetImageManifest(common.TestFolder + "image-manifest.json")
+		assert.NoError(t, err)
+		assert.Equal(t, expectedOCI, res)


Many thanks! ❤️

openshift-ci · 2025-02-27T11:15:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: r4f4, sherine-k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sherine-k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lmzuccarelli · 2025-03-06T10:54:20Z

/retitle no-jira: v2/manifest/oci-manifest: refactor and dedup the code

openshift-ci-robot · 2025-03-06T10:54:26Z

@r4f4: This pull request explicitly references no jira issue.

In response to this:

Description

Introduce a generic json/yaml parser functions

Refactor and dedup code in oci-manifest.go

Fixed unit tests to match error and/or return value

Add more unit tests

Github / Jira issue:

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

New feature (non-breaking change which adds functionality)

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code Improvements (Refactoring, Performance, CI upgrades, etc)

Internal repo assets (diagrams / docs on github repo)

This change requires a documentation update on openshift docs

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Expected Outcome

Please describe the outcome expected from the tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

lmzuccarelli · 2025-03-06T11:11:04Z

Verified locally using custom CICD for any v2 regression

M2D
D2M
M2M
Delete generate
Delete

lmzuccarelli · 2025-03-06T11:15:36Z

/lgtm

aguidirh · 2025-03-06T11:31:00Z

/hold

aguidirh

Thanks for this PR @r4f4, it will help a lot to maintain the code in the long term.

I really like the new package, it will reduce a lot the duplications.

I added only a nit comment. LGTM as well.

aguidirh · 2025-03-06T11:29:19Z

v2/internal/pkg/clusterresources/clusterresources.go

-		return obj, fmt.Errorf("error during CatalogSource generation using template: error reading targetCatalogSourceTemplate file %s: %v", templateFile, err)
-	}
-	err = yaml.Unmarshal(bytesRead, &obj)
+	obj, err = common.ParseYamlFile[ofv1alpha1.CatalogSource](templateFile)


Thanks, this will help a lot avoiding the duplication of Unmarshal/Marshal

aguidirh · 2025-03-06T11:38:49Z

v2/internal/pkg/common/parse_test.go

@@ -0,0 +1,59 @@
+package common


Thanks for adding this package @r4f4.

Nit: since the package only contains parsers, what about renaming it to something like parser (or something else in case of conflict) ?

In this way we follow the package names guidelines (https://go.dev/blog/package-names)

Reading a file and parsing the data as json/yaml into a variable of type `T` is a very common task. By having a generic function for them, we make sure the error messages are consistent across the code base.

I have also added more unit tests.

openshift-ci · 2025-03-06T23:02:32Z

@r4f4: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

aguidirh · 2025-03-07T10:46:21Z

Thanks a lot @r4f4, this refactoring is going to help a lot.

aguidirh · 2025-03-07T10:46:28Z

/unhold

aguidirh · 2025-03-07T10:46:33Z

/lgtm

openshift-bot · 2025-03-07T15:40:42Z

[ART PR BUILD NOTIFIER]

Distgit: oc-mirror-plugin
This PR has been included in build oc-mirror-plugin-container-v4.19.0-202503071410.p0.g2293058.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci bot requested review from aguidirh and lmzuccarelli February 26, 2025 18:37

r4f4 force-pushed the oci-manifest-refactor branch 2 times, most recently from a6b854f to d699a4f Compare February 27, 2025 09:25

sherine-k approved these changes Feb 27, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2025

r4f4 force-pushed the oci-manifest-refactor branch from d699a4f to e9ffe9e Compare February 27, 2025 17:08

openshift-ci bot changed the title ~~v2/manifest/oci-manifest: refactor and dedup the code~~ no-jira: v2/manifest/oci-manifest: refactor and dedup the code Mar 6, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 6, 2025

openshift-ci bot assigned lmzuccarelli Mar 6, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2025

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2025

aguidirh reviewed Mar 6, 2025

View reviewed changes

r4f4 added 4 commits March 6, 2025 20:54

v2/common: introduce generic json/yaml parser functions

0d492d3

Reading a file and parsing the data as json/yaml into a variable of type `T` is a very common task. By having a generic function for them, we make sure the error messages are consistent across the code base.

v2/manifest/oci-manifest: refactor and dedup the code

c6ddde4

I have also added more unit tests.

v2: use yaml/json parser functions

895d185

v2: move parsing functions to their own package.

23951eb

r4f4 force-pushed the oci-manifest-refactor branch from e9ffe9e to 23951eb Compare March 6, 2025 19:54

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2025

openshift-ci bot assigned aguidirh Mar 7, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2025

openshift-merge-bot bot merged commit 2293058 into openshift:main Mar 7, 2025
7 checks passed

aguidirh mentioned this pull request Nov 28, 2025

[release-4.18] OCPBUGS-66051: Remove empty status field from generated IDMS/ITMS files #1319

Open

		@@ -0,0 +1,63 @@
		//nolint:ireturn // generic T should be fine to return here
		package common

no-jira: v2/manifest/oci-manifest: refactor and dedup the code #1098

no-jira: v2/manifest/oci-manifest: refactor and dedup the code #1098

Uh oh!

Conversation

r4f4 commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Expected Outcome

Uh oh!

sherine-k left a comment

Choose a reason for hiding this comment

Uh oh!

sherine-k Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sherine-k Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

sherine-k Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 27, 2025

Uh oh!

lmzuccarelli commented Mar 6, 2025

Uh oh!

openshift-ci-robot commented Mar 6, 2025

Description

Type of change

How Has This Been Tested?

Expected Outcome

Uh oh!

lmzuccarelli commented Mar 6, 2025

Uh oh!

lmzuccarelli commented Mar 6, 2025

Uh oh!

aguidirh commented Mar 6, 2025

Uh oh!

aguidirh left a comment

Choose a reason for hiding this comment

Uh oh!

aguidirh Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

aguidirh Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

r4f4 Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Mar 6, 2025

Uh oh!

aguidirh commented Mar 7, 2025

Uh oh!

aguidirh commented Mar 7, 2025

Uh oh!

aguidirh commented Mar 7, 2025

Uh oh!

Uh oh!

openshift-bot commented Mar 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

r4f4 commented Feb 26, 2025 •

edited

Loading