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

Automatically copy module_defaults when their associated modules are copied #1400

Merged
merged 8 commits into from
Aug 7, 2019

Conversation

dralley
Copy link
Contributor

@dralley dralley commented Jul 10, 2019

No description provided.

@pep8speaks
Copy link

pep8speaks commented Jul 10, 2019

Hello @dralley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-07 16:41:50 UTC

@dralley dralley force-pushed the fix-modular-depsolv2 branch 7 times, most recently from b003478 to de30962 Compare July 16, 2019 13:23
@dralley dralley force-pushed the fix-modular-depsolv2 branch 8 times, most recently from ea99f89 to ee95129 Compare July 23, 2019 02:53
@dralley dralley force-pushed the fix-modular-depsolv2 branch 2 times, most recently from e133f89 to a027769 Compare July 30, 2019 16:14
@dralley dralley force-pushed the fix-modular-depsolv2 branch 2 times, most recently from 10eabec to c045a6a Compare July 30, 2019 18:09
@dralley
Copy link
Contributor Author

dralley commented Aug 1, 2019

This PR is currently failing a couple of smash tests because it's pulling in dependencies that were being not being pulled in by the code previously (but I think it's correct in doing so).

There's 5 tests failing but all are in the ModularErrataCopyTestCase. FWIW I only looked into the first of them directly but they all run the same code with different parameters so I assume the issue is essentially the same for all of them.

The tests check the total number of units being copied, which is obviously different now that module-defaults are being copied.

The errata being tested by ModularErrataCopyTestCase pulls in kangaroo-0.3-1.noarch.rpm and duck-0.7-1.noarch.rpm, neither of which have any RPM dependencies. However, they are both part of modules, and both modules have module-defaults, so they get copied along.

@dralley
Copy link
Contributor Author

dralley commented Aug 1, 2019

Tests pass with this PR pulp/Pulp-2-Tests#211

@nixocio
Copy link

nixocio commented Aug 2, 2019

Is there a related pulp issue?

@dralley
Copy link
Contributor Author

dralley commented Aug 3, 2019

Pulp issue linked in commits: https://pulp.plan.io/issues/5071

Test issue to link it to (or make a subtask for): https://pulp.plan.io/issues/5055#note-2

# TODO: Since we're copying the module default metadata as-is without modification or
# regeneration, do we need to add a "requires" for every stream for which there is a
# profile? Is it OK to have profiles for module streams that aren't present or is that
# as terrible an idea as it sounds?
Copy link
Member

Choose a reason for hiding this comment

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

@dralley , I think it's ok to have profiles for module streams that aren't present, they are just used for lookups, AFAIK

Copy link
Member

Choose a reason for hiding this comment

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

I'm +1 to to add a "requires" for every stream for which there is a profile

@goosemania
Copy link
Member

Where is a conflict with multiple module-defaults solved?
E.g. I copy a module which already has a module-defaults in the destination repo.
Will the solver treat it as satisfied dependency in the destination repo and won't try to copy module-defaults from the source repo at all?
IIRC, the proper conflict resolution for module-defaults is provided by libmodulemd.

@dralley
Copy link
Contributor Author

dralley commented Aug 7, 2019

@goosemania It's not implemented - and yes, Steven Gallagher said basically the same thing. Except I'm not sure if libmodulemd provides actual "conflict resolution" or if it just provides validation that metadata is/is not acceptable. My recollection of our discussion was that he thought if validation failed in libmodulemd we should just fail the whole task and not try to resolve conflicts, at least not until we're asked to have that feature, because it would be somewhat fraught with peril. That's probably a discussion we should have w/ product folks, but in my mind it's also worthy of a separate issue and PR.

Copy link
Member

@goosemania goosemania 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! :octocat: :octocat:

Libsolv does need these, so we have to provide fake ones, not just an
empty string.

re: #5071
https://pulp.plan.io/issues/5071
This is unnecessary, according to Igor.

re: #5071
https://pulp.plan.io/issues/5071
Change the name of module-default solvables to follow convention. Make
sure module-defaults without a default stream aren't setting streams as
defaults. Change the way provides are set up, as there should be no such
thing as "module-default(name:stream)" since only one module-default
exists per-module.

re: #5071
https://pulp.plan.io/issues/5071
Allow it to copy modules also. Ideally, we would do more refactoring to
make this less hacky, but unless we change the scope of my work I would
prefer to change as little as possible in that regard.

re: #5071
https://pulp.plan.io/issues/5071
It's supposed to be possible, but it's problematic in practice.

re: #5071
https://pulp.plan.io/issues/5071
I think Igor was wrong there, empty strings are fine.

re: #5071
https://pulp.plan.io/issues/5071
@dralley dralley merged commit a2380f6 into pulp:2-master Aug 7, 2019
@dralley dralley deleted the fix-modular-depsolv2 branch August 7, 2019 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants