Conversation
e578fef to
326614a
Compare
974629e to
a3577ca
Compare
ArtifactInfo
b64c4be to
3d8a44e
Compare
652fa41 to
2eee10e
Compare
| import pytest | ||
|
|
||
| from tmt.steps.prepare.artifact.providers.copr_repository import CoprRepositoryProvider | ||
|
|
There was a problem hiding this comment.
Seems that the class is not used. Interesting that it was not caught by ruff. Ah, we have it disabled:
There was a problem hiding this comment.
Shall I wait for it to be fixed in the above pr?
This import is needed to register the provider in the plugin registry. Just realised after trying to remove it.
There was a problem hiding this comment.
That is quite funky. There is a load plugin method, but I did not look into how that works. Do you have the bandwitdh to check it?
There was a problem hiding this comment.
Maybe to unblock this - we can look at adding this to the housekeeping sprint?
There was a problem hiding this comment.
ok... so did a bit of digging.
there's a _explore_packages() hidden in explore() that loads plugins on start... however this does not get called automagically for unit tests, we need to explicitly call it to load the providers. Added a fixture for this. See: 0119fe4
Maybe we could try lazy-loading in the plugin registry (so it auto-discovers on first access)? Obviously as a separate change though.
There was a problem hiding this comment.
Fine for now, there are some pytest constructs that can avoid adding it manually to each test (define a fixture to be used under a conftest scope), but we can get to that later.
There was a problem hiding this comment.
'm a bit confused now - isnt that what I have pushed? a fixture _load_plugins() and making artifact_provider() depend on it. Am i missing something?
f3734d2 to
a5b4976
Compare
|
/packit build |
a5b4976 to
8b98406
Compare
LecrisUT
left a comment
There was a problem hiding this comment.
Just a few other minor comments, otherwise it's fine from my side. Could reorganize where the class definitions are, but I will leave it to you to decide when to do so.
cb25be9 to
9024859
Compare
|
It's a bit late for the release and this change does not directly affect users. |
0119fe4 to
646c154
Compare
646c154 to
4a9f51a
Compare
An effort towards resolving: #4055 (part-1)
following changes made:
ArtifactInfonow removedArtifactInfoto what was discussed during the artifact plugin design sync (id,version,name,location,provider). The__str__of this container represents:<provider-id>-<nvra>Versionobject and its subclass (RpmVersionwith its factory methods) to representnvra&nevraconstructionRefactoredartifactspropeorty toget_installable_artifactsmethod. Reason being only persist information about artifacts that would be downloaded i.e. list from providers that "download" and not "enable".ArtifactInfoTgeneric since see point 1.couple more to finish before moving to review:
1. moveThe providers shall be responsible to createArtifactInfoto someplace else visible to package_managers too.ArtifactInfoobjects.