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

rpmmd: add module_hotfixes flag to RepoConfig #284

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

ezr-ondrej
Copy link
Contributor

@ezr-ondrej ezr-ondrej commented Nov 28, 2023

Adds module_hotfixes flag to all repo types so it can be used during osbuild. This enables users to disable modularity filtering on specific repositories.

relates to: osbuild/osbuild-composer#3821

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.

Needs some tests added, and current hash values need updating :)

Copy link
Member

@ondrejbudai ondrejbudai 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, a quick test at least for the dnfjson conversion code is indeed quite crucial.

@ezr-ondrej
Copy link
Contributor Author

Needs some tests added, and current hash values need updating :)

The aim here was not to change the hash if the flag is not set, I've clearly failed.
I guess we can just completely disregard False and only pass the flag if it's True.

@ezr-ondrej ezr-ondrej force-pushed the allow_module_hotfixes_flag branch 3 times, most recently from e9d57c9 to a7913fa Compare December 1, 2023 11:26
@ezr-ondrej
Copy link
Contributor Author

Alright, we are 🍏 here, I've added a test, but quite a simle one, if you'd have an idea what else we should cover, let me know pls, so we can get this in 🙏

@miabbott
Copy link

miabbott commented Dec 1, 2023

nit: the commit message needs s/hotifxes/hotfixes/ in the title

@teg teg enabled auto-merge December 2, 2023 00:21
@teg teg disabled auto-merge December 2, 2023 00:21
@ezr-ondrej ezr-ondrej changed the title rpmmd: add module_hotifxes flag to RepoConfig rpmmd: add module_hotfixes flag to RepoConfig Dec 2, 2023
@ezr-ondrej
Copy link
Contributor Author

nit: the commit message needs s/hotifxes/hotfixes/ in the title

Hehe, true, indeed, thx, good catch 🙏

mvo5
mvo5 previously approved these changes Dec 4, 2023
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! This looks fine AFAICT (take it with a grain of salt, I'm relatively new to this codebase) as a short term fix. I have some inline comments and if my assumptions are correct I would love to have a followup for them - I'm happy to volunteer if you are busy of course :)

pkg/dnfjson/dnfjson.go Outdated Show resolved Hide resolved
pkg/rpmmd/repository.go Show resolved Hide resolved
pkg/rpmmd/repository.go Outdated Show resolved Hide resolved
pkg/rpmmd/repository.go Outdated Show resolved Hide resolved
@ezr-ondrej ezr-ondrej force-pushed the allow_module_hotfixes_flag branch 3 times, most recently from b3cdcf2 to ad8372f Compare December 4, 2023 13:27
mvo5
mvo5 previously approved these changes Dec 4, 2023
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you for this and also for the followup. This looks very nice and straightforward now.

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. However, I found one problem which I consider a blocker.

In addition to that, I realized that you didn't modify the dnf-json copy in this repository, which then makes the newly added repo config value be ignored by it in testing. Please modify it in the same way as you did in the osbuild-composer PR.

pkg/dnfjson/dnfjson.go Outdated Show resolved Hide resolved
Adds module_hotfixes flag to all repo types so it can be used during osbuild.
This enables users to disable modularity filtering on specific repositories.
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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, thanks!

@ezr-ondrej ezr-ondrej added this pull request to the merge queue Dec 4, 2023
Merged via the queue into osbuild:main with commit f8afbe1 Dec 4, 2023
9 checks passed
@ezr-ondrej ezr-ondrej deleted the allow_module_hotfixes_flag branch December 4, 2023 20:59
@thozza thozza mentioned this pull request Dec 5, 2023
thozza added a commit to thozza/osbuild-images that referenced this pull request Dec 5, 2023
Just testing if our current CI tests would have caught
osbuild#284 (comment)

Signed-off-by: Tomáš Hozza <[email protected]>
mvo5 added a commit to mvo5/images that referenced this pull request Dec 5, 2023
This is a regression test for an issue found during the review
of PR#284 [0]. The issue was that
```
	ModuleHotfixes *bool    `json:"module_hotfixes"`
```
lacked the required `omitempty` which may cause issues
with dnfjson.

As a quick and easy way to prevent this in the future
this commit adds a small test that ensures that we notice
any missing `omitempty`.

Note that the `rpmmd` part is not strictly needed but it
feels more correct to do the check in both places that
use a repoConfig.

[0] osbuild#284 (comment)
mvo5 added a commit to mvo5/images that referenced this pull request Dec 5, 2023
This is a regression test for an issue found during the review
of PR#284 [0]. The issue was that
```
	ModuleHotfixes *bool    `json:"module_hotfixes"`
```
lacked the required `omitempty` which may cause issues
with dnfjson.

As a quick and easy way to prevent this in the future
this commit adds a small test that ensures that we notice
any missing `omitempty`.

Note that the `rpmmd` part is not strictly needed but it
feels more correct to do the check in both places that
use a repoConfig.

[0] osbuild#284 (comment)
mvo5 added a commit to mvo5/images that referenced this pull request Dec 5, 2023
This is a regression test for an issue found during the review
of PR#284 [0]. The issue was that
```
	ModuleHotfixes *bool    `json:"module_hotfixes"`
```
lacked the required `omitempty` which may cause issues
with dnfjson.

As a quick and easy way to prevent this in the future
this commit adds a small test that ensures that we notice
any missing `omitempty`.

Note that the `rpmmd` part is not strictly needed but it
feels more correct to do the check in both places that
use a repoConfig.

[0] osbuild#284 (comment)
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2023
This is a regression test for an issue found during the review
of PR#284 [0]. The issue was that
```
	ModuleHotfixes *bool    `json:"module_hotfixes"`
```
lacked the required `omitempty` which may cause issues
with dnfjson.

As a quick and easy way to prevent this in the future
this commit adds a small test that ensures that we notice
any missing `omitempty`.

Note that the `rpmmd` part is not strictly needed but it
feels more correct to do the check in both places that
use a repoConfig.

[0] #284 (comment)
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.

6 participants