setuptargetrepos actor code cleanup #301
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bocekm
force-pushed
the
target_repos_actor_cleanup
branch
3 times, most recently
from
67e19f2
to
0e3c4c6
Compare
bocekm
changed the title
bocekm
force-pushed
the
target_repos_actor_cleanup
branch
from
0e3c4c6
to
09b1c2a
Compare
alexandrepossebom
suggested changes
Aug 16, 2019
bocekm
force-pushed
the
target_repos_actor_cleanup
branch
from
09b1c2a
to
07bcbbe
Compare
alexandrepossebom
suggested changes
Aug 28, 2019
bocekm
force-pushed
the
target_repos_actor_cleanup
branch
from
07bcbbe
to
d6f3a5a
Compare
bocekm
commented
Sep 12, 2019
bocekm
force-pushed
the
target_repos_actor_cleanup
branch
2 times, most recently
from
3e6f9f7
to
3b0f280
Compare
|
@asilveir, ready for re-review. |
|
leapp-ci build |
bocekm
commented
Sep 16, 2019
repos/system_upgrade/el7toel8/actors/targetuserspacecreator/libraries/userspacegen.py
Show resolved
Hide resolved
bocekm
force-pushed
the
target_repos_actor_cleanup
branch
from
3b0f280
to
e804a95
Compare
bocekm
force-pushed
the
target_repos_actor_cleanup
branch
from
e804a95
to
b0b125e
Compare
|
Rebased and conflicts resolved. |
alexandrepossebom
approved these changes
Sep 23, 2019
|
Tested and working. |
pirat89
reviewed
Sep 25, 2019
| @@ -6,6 +6,7 @@ class RepositoriesSetupTasks(Model): | |||
| """ | |||
| Information about repositories that must be managed in order to complete upgrade process. | |||
|
|
|||
| !!! THIS MODEL IS OBSOLETE. Use for example CustomTargetRepositories instead. !!! | |||
There was a problem hiding this comment.
I do not like to see the model is kind of obsoleted. I can imagine in future that based on checks, there will be case a specific repository needs to be enabled / disabled (talking about RHEL repos).
I am fun of Tasks suffix as this is kind of self explaining name we could use across many problems we would like to handle on one place. But the approach is questionable. Let's see what will be the way in future.
pirat89
reviewed
Sep 25, 2019
|
|
||
| enabled_repos = set() | ||
| def process(self): | ||
| self.get_custom_repos() |
There was a problem hiding this comment.
Actually, it returns now rhel repositories. not custom:
p self.custom_repos
[<leapp.models.targetrepositories.CustomTargetRepository object at 0x7f115f1ed3d0>, <leapp.models.targetrepositories.CustomTargetRepository object at 0x7f115f1ed410>, <leapp.models.targetrepositories.CustomTargetRepository object at 0x7f115f1ed450>]
(Pdb) p self.custom_repos[0].name
None
(Pdb) p self.custom_repos[0].baseurl
None
(Pdb) p self.custom_repos[0].enabled
True
(Pdb) p self.custom_repos[0].repoid
u'rhel-8-for-x86_64-baseos-rpms'
(Pdb) p self.custom_repos[1].repoid
u'codeready-builder-for-rhel-8-x86_64-rpms'
(Pdb) p self.custom_repos[2].repoid
u'rhel-8-for-x86_64-appstream-rpms'
...Expected only two customrepos (produced by custom actor) with APPSTREAM and BASEOS names.
pirat89
reviewed
Sep 25, 2019
| @@ -406,4 +407,4 @@ def produce_messages(tasks): | |||
| to_remove=to_remove_pkgs)) | |||
|
|
|||
| if to_enable_repos: | |||
| api.produce(RepositoriesSetupTasks(to_enable=to_enable_repos)) | |||
| api.produce(CustomTargetRepositories(repos=[CustomTargetRepository(repoid=repo) for repo in to_enable_repos])) | |||
There was a problem hiding this comment.
that's the thing. You are presenting RH repos as custom which is wrong. You are changing here the meaning of CustomTargetRepositories model at all. (this is not the only issue in the PR as original msgs CustomTargetRepository messages are not processed.)
pirat89
added a commit
to pirat89/leapp-repository
that referenced
this pull request
Sep 25, 2019
This reverts commit 47188e3. The commit brings several issues and I do not have power to fix it now correctly. I am reverting so it can be fixed properly in future without blocking the others now.
vinzenz
pushed a commit
that referenced
this pull request
Sep 25, 2019
This reverts commit 47188e3. The commit brings several issues and I do not have power to fix it now correctly. I am reverting so it can be fixed properly in future without blocking the others now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To changelog:
Remove RepositoriesSetupTasks which was kinda duplicate to the CustomTargetRepository.
Resolves #233.