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

targetuserspacecreator: Refactor gathering of repositories #973

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vinzenz
Copy link
Member

@vinzenz vinzenz commented Oct 12, 2022

This patch unifies the collection of repositories.

Signed-off-by: Vinzenz Feenstra [email protected]

@vinzenz vinzenz requested a review from pirat89 October 12, 2022 09:51
@packit-as-a-service
Copy link

Your git-forge project github.com/oamg/leapp-repository has permissions to build in @oamg/leapp Copr project configured in Packit. However, we migrated to the solution where you can configure the allowed git-forge projects in Copr yourself and will remove the configuration in Packit for the allowed projects soon. Therefore, please, add this git-forge project github.com/oamg/leapp-repository to Packit allowed forge projectsin the Copr project settings.

@github-actions
Copy link

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please to notify leapp developers of review request
  • /packit copr-build to submit a public copr build using packit

To launch regression testing public members of oamg organization can leave the following comment:

  • /rerun to schedule basic regression tests using this pr build and leapp*master* as artifacts
  • /rerun 42 to schedule basic regression tests using this pr build and leapp*PR42* as artifacts
  • /rerun-all to schedule all tests (including sst) using this pr build and leapp*master* as artifacts
  • /rerun-all 42 to schedule all tests (including sst) using this pr build and leapp*PR42* as artifacts

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra.

@vinzenz vinzenz force-pushed the repository-collection-refactor branch 7 times, most recently from 8f27e71 to 6e293b3 Compare October 31, 2022 14:05
@vinzenz
Copy link
Member Author

vinzenz commented Oct 31, 2022

/rerun

@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/4998669

@github-actions
Copy link

Testing Farm request for RHEL-8.6-rhui/4998669 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.7.0-Nightly/4998669 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.6.0-Nightly/4998669 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-ZStream/4998669 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-rhui/4998669 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-ZStream/4998669 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@vinzenz vinzenz force-pushed the repository-collection-refactor branch from 6e293b3 to aeeed23 Compare October 31, 2022 22:16
@vinzenz vinzenz force-pushed the repository-collection-refactor branch 3 times, most recently from 96b2ac7 to f904598 Compare November 8, 2022 12:01
@vinzenz vinzenz force-pushed the repository-collection-refactor branch from f904598 to bf6e1ab Compare December 12, 2022 13:51
@vinzenz vinzenz changed the title WIP: targetuserspacecreator: Refactor gathering of repositories targetuserspacecreator: Refactor gathering of repositories Dec 12, 2022
@vinzenz vinzenz force-pushed the repository-collection-refactor branch 5 times, most recently from fd79fd1 to c1ae302 Compare December 13, 2022 08:30
@vinzenz vinzenz force-pushed the repository-collection-refactor branch from c1ae302 to 4958ac3 Compare December 13, 2022 08:36
Copy link
Member

@MichalHe MichalHe left a comment

Choose a reason for hiding this comment

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

I've given the patch a first pass, and one more pass will likely follow. Separating the manipulation of repositories into a compact class allowing to query the needed information is a very elegant concept. As the introduced class is likely to stick, I would argue to change the names of its fields to be more descriptive. For example, the _RequestedRepoids.mapped field has a name suggesting almost nothing about its semantics.

Comment on lines +86 to +87
def duplicated_repoids(self):
return {k: v for k, v in self.mapped.items() if len(v) > 1}
Copy link
Member

Choose a reason for hiding this comment

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

The property name suggests that it contains a collection of repoids that are duplicated, whereas a dictionary is returned. Every use of this property access to the values of the property requests only its keys().

Copy link
Member Author

@vinzenz vinzenz Jan 6, 2023

Choose a reason for hiding this comment

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

Well it is right now, the point is that you can now report which repo files do contain the duplications. But this would be a follow up improvement. And it is a collection, key = repoid, value = list of repos with the same repo id)

Copy link
Member

Choose a reason for hiding this comment

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

list of repos here means the list of repository files

This patch unifies the collection of repositories.

Signed-off-by: Vinzenz Feenstra <[email protected]>
@vinzenz vinzenz force-pushed the repository-collection-refactor branch from 4958ac3 to c18e9c6 Compare January 6, 2023 07:56
@@ -26,7 +26,7 @@ def asbool(x):
return RepositoryData(**prepared)


def parse_repofile(repofile):
def parse_repofile(repofile, rfile=None, kind='custom'):
Copy link
Member

Choose a reason for hiding this comment

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

extend the docstring to cover new params

@pirat89
Copy link
Member

pirat89 commented Jan 11, 2023

/packit build

@pirat89
Copy link
Member

pirat89 commented Jan 18, 2023

@vinzenz do you think you could finish this one during this week or want a help? :)

@@ -169,13 +168,6 @@ def get_available_repo_ids(context):
)

repofiles = repofileutils.get_parsed_repofiles(context)
Copy link
Member

Choose a reason for hiding this comment

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

this call leads to the situation when all discovered repositories have the custom kind set, so repositories inside RHSMInfo msg are marked as custom. Discussed with @vinzenz , he will check whether the logic of selecting (custom, rhsm, rhui) kind could be moved into the library instead of having it in the private library of the actor.

Copy link
Member

@pirat89 pirat89 left a comment

Choose a reason for hiding this comment

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

see the comments above. also unit-tests needs to be fixed.

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.

3 participants