Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factor out common RHEL parts and RHEL-10 from RHEL-9 (COMPOSER-2185) #529

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

thozza
Copy link
Member

@thozza thozza commented Mar 19, 2024

This is the first step in defining RHEL distros by reusing most of the code which is not release-specific. Each release will then define its package sets, partition tables, image configurations. The rest will be shared among distros.

I factored out the "common" RHEL code into rhel package, which is now used by rhel9. Subsequently, I factored out rhel10 out of rhel9 and reverted the conditionals in rhel9 distro code which were necessary to incorporate rhel10.

I verified that no el9 / c9s / el10 / c10s manifests have been changed by these changes.

@supakeen
Copy link
Member

Very nice, I believe this will also fix #491? I'll do a review tomorrow.

@thozza thozza marked this pull request as ready for review March 20, 2024 08:23
@thozza thozza changed the title Factor out common RHEL parts from RHEL-9 into rhel_common Factor out common RHEL parts and RHEL-10 from RHEL-9 Mar 20, 2024
@thozza thozza changed the title Factor out common RHEL parts and RHEL-10 from RHEL-9 Factor out common RHEL parts and RHEL-10 from RHEL-9 (COMPOSER-2185) Mar 20, 2024
@thozza thozza force-pushed the el10 branch 2 times, most recently from edae9f1 to b14b2be Compare March 20, 2024 11:02
@thozza thozza marked this pull request as draft March 20, 2024 11:23
@thozza thozza marked this pull request as ready for review March 20, 2024 11:39
@thozza
Copy link
Member Author

thozza commented Mar 20, 2024

Very nice, I believe this will also fix #491? I'll do a review tomorrow.

Not sure if I fully understand your intention with #491. But I'm happy to amend the PR to incorporate it.

@thozza
Copy link
Member Author

thozza commented Mar 20, 2024

Snyk is complaining about an existing thing in rhel9 distro. But due to moving the whole distro to a new directory, it thinks that it is a newly introduced code.

pkg/distro/fedora/images.go Outdated Show resolved Hide resolved
@thozza thozza marked this pull request as draft March 22, 2024 09:56
@thozza thozza force-pushed the el10 branch 4 times, most recently from 52d3435 to d15a968 Compare March 27, 2024 14:06
@thozza thozza marked this pull request as ready for review March 27, 2024 14:06
@thozza thozza marked this pull request as draft March 27, 2024 15:15
@thozza thozza force-pushed the el10 branch 2 times, most recently from a91df58 to e0efc38 Compare March 27, 2024 16:02
@thozza thozza marked this pull request as ready for review March 27, 2024 16:02
@thozza thozza requested a review from bcl March 27, 2024 16:05
@thozza
Copy link
Member Author

thozza commented Mar 27, 2024

The PR is again ready for review.

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. I didn't verify every single copy of the image types line-by-line since the test manifests haven't changed.

Two minor comments in-line: a file with a mistyped name and a question about the NewImageType() function.

SUPER NITPICK: One more thought: Could the common parts be moved to pkg/distro/rhel/ instead of being under pkg/distro/rhel/rhel_common? I'm not sure I like the repetition in the path (though, unavoidable I guess), the name with the _, and I kinda like the idea of the common bits being one level above the version-specific code.

So instead of:

pkg/distro/
└── rhel
    ├── rhel10
    ├── rhel9
    └── rhel_common

we'd have:

pkg/distro
└── rhel
    ├── rhel10
    └── rhel9

pkg/distro/rhel/rhel9/optioins.go Outdated Show resolved Hide resolved
pkg/distro/rhel/rhel_common/imagetype.go Outdated Show resolved Hide resolved
@thozza
Copy link
Member Author

thozza commented Mar 28, 2024

Looks good overall. I didn't verify every single copy of the image types line-by-line since the test manifests haven't changed.

Two minor comments in-line: a file with a mistyped name and a question about the NewImageType() function.

Thanks a lot for the review!

SUPER NITPICK: One more thought: Could the common parts be moved to pkg/distro/rhel/ instead of being under pkg/distro/rhel/rhel_common? I'm not sure I like the repetition in the path (though, unavoidable I guess), the name with the _, and I kinda like the idea of the common bits being one level above the version-specific code.

So instead of:

pkg/distro/
└── rhel
    ├── rhel10
    ├── rhel9
    └── rhel_common

we'd have:

pkg/distro
└── rhel
    ├── rhel10
    └── rhel9

I don't have a strong opinion on this. I'm happy to move the common code to pkg/distro/rhel...

@thozza
Copy link
Member Author

thozza commented Mar 28, 2024

I fixed typos in filenames and moved rhel_common under pkg/distro/rhel and renamed it to rhel.

@thozza thozza requested a review from achilleas-k March 28, 2024 13:10
achilleas-k
achilleas-k previously approved these changes Mar 28, 2024
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

bcl
bcl previously approved these changes Mar 29, 2024
Copy link
Contributor

@bcl bcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@achilleas-k
Copy link
Member

@thozza Tiny merge conflict :(

As the first step towards defining el10 and el9 in without completely
forking the `rhel9` distro, create a common `rhel` directory and move
`rhel9` under it. The idea is to then extract the parts that should be
shared across RHEL distros under `rhel` package and export necessary
structs and functions from it and make use of them.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
Create new package under `distro/rhel`, called `rhel`. The intention
is to factor out all code which should be common across RHEL versions
into this new package. Specific RHEL version distro implementations
will then use this code and own mostly their package set and
configuration.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
While extracting RHEL-10 from RHEL-9, I noticed that the test passes
even if the listed image types are not supported by the distro, which
didn't seem right. Fix that.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
And base it on the common `rhel` package.

Signed-off-by: Tomáš Hozza <thozza@redhat.com>
@thozza thozza added this pull request to the merge queue Apr 2, 2024
Merged via the queue into osbuild:main with commit b0cebe8 Apr 2, 2024
13 of 16 checks passed
schuellerf added a commit to schuellerf/osbuild-composer that referenced this pull request Apr 4, 2024
schuellerf added a commit to schuellerf/osbuild-composer that referenced this pull request Apr 5, 2024
schuellerf added a commit to schuellerf/osbuild-composer that referenced this pull request Apr 8, 2024
schuellerf added a commit to schuellerf/osbuild-composer that referenced this pull request Apr 22, 2024
schuellerf added a commit to schuellerf/osbuild-composer that referenced this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants