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

Add support for stream overrides in configurations #601

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sgallagher
Copy link
Collaborator

Fixes: #599

Signed-off-by: Stephen Gallagher sgallagh@redhat.com

sgallagher and others added 2 commits July 26, 2022 19:53
Fixes: fedora-modularity#599

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
Signed-off-by: Libmodulemd CI <github-actions@github.com>
@sgallagher sgallagher requested a review from ppisar July 26, 2022 23:54
@ppisar ppisar self-assigned this Jul 27, 2022
@ppisar
Copy link
Collaborator

ppisar commented Jul 28, 2022

I reviewed this patch. I found these minor omissions which should be checked or implemented:

  • Increase libmodulemd version
  • Merge source code formatting commit
  • Handle stream override in modulemd_build_config_equals()
  • Handle stream override in modulemd_build_config_compare()

But I really don't like how it breaks modulemd_packager_v3_to_stream_v2() and modulemd_packager_v3_to_stream_v2_ext() (which you adjusted tests for):

The patch changes stream as a side effect in copy_packager_v3_buildconfig_to_stream_v2(). As a result modulemd_packager_v3_to_stream_v2() will assume a stream override of the latest buildconfig as a stream. It breaks modulemd_packager_v3_to_stream_v2() in the opposite direction than it's already broken in regard of buildopts. modulemd_packager_v3_to_stream_v2() adopts the first buildopts. And now it adopts the latest stream.

I understand that it stems from the ill-design of modulemd_packager_v3_to_stream_v2_ext() which produces a single modulemd-v2 object instead of a list the objects, an objects for each modulemd-packager-v3 buildconfig. I would swallow it as a necessary collateral damage. But I don't like that the damage is not in accord with buildopts. Could you please change the patch to:

  • Either raise an error if the overrides differ among buildconfigs,
  • or use an override of the first buildconfig as it is done with buildopts?
  • Or ignore the override completely?

And document handling of the stream override in these conversion functions?

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.

Packager v3: Extend configurations to allow stream renaming
2 participants