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.

Sign up for GitHub

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

Jump to bottom

Fix ModelViolationError while parsing repo files #1266

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

Fix ModelViolationError while parsing repo files #1266

wants to merge 1 commit into from
+78 −3

Conversation

tomasfratrik
Copy link
Member

@tomasfratrik tomasfratrik commented Jul 17, 2024 •
edited
Loading

This error occurs when repo file has invalid definition, specifically the 'name' entry of the config file.

After this, we get rid of the traceback error, and the following errors are shown

2024-06-17 13:35:00.974356 [ERROR] Actor: system_facts
Message: Invalid repository definition: invalid-repo in: /etc/yum.repos.d/invalid.repo: The value of "name" field is None, but this is not allowed
Summary:
    Hint: For more directions on how to resolve the issue, see: https://access.redhat.com/solutions/6969001.
2024-06-17 13:35:01.545642 [ERROR] Actor: repositories_blacklist
Message: Actor didn't receive required messages: RepositoriesFact

Jira: RHEL-19249

@github-actions 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 @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
However, here are additional useful commands for packit:

  • /packit test to re-run manually the default tests
  • /packit retest-failed to re-run failed tests manually
  • /packit test oamg/leapp#42 to run tests with leapp builds for the leapp PR#42 (default is latest upstream - master - build)

Note that first time contributors cannot run tests automatically - they need to be started by a reviewer.

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

  • /packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
  • /packit test --labels beaker-minimal-8.10to9.4,kernel-rt-8.10to9.4 to schedule kernel-rt and beaker-minimal test sets for 8.10->9.4 upgrade path

See other labels for particular jobs defined in the .packit.yaml file.

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 contact leapp-infra.

@tomasfratrik tomasfratrik force-pushed the fix-modelviolation-error branch 3 times, most recently from 3d49f24 to b294a98 Compare July 17, 2024 12:03
@tomasfratrik
Copy link
Member Author

/packit retest-failed

2 similar comments
@matejmatuska
Copy link
Member

/packit retest-failed

@matejmatuska
Copy link
Member

/packit retest-failed

matejmatuska
matejmatuska requested changes Jul 19, 2024
View reviewed changes
Copy link
Member

@matejmatuska matejmatuska left a comment

Choose a reason for hiding this comment

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

The overall approach is ok, but there are some problems.

repos/system_upgrade/common/actors/systemfacts/libraries/systemfacts.py Outdated Show resolved Hide resolved
repos/system_upgrade/common/actors/systemfacts/libraries/systemfacts.py Outdated Show resolved Hide resolved
@tomasfratrik tomasfratrik marked this pull request as ready for review July 24, 2024 07:49
matejmatuska
matejmatuska requested changes Aug 2, 2024
View reviewed changes
Copy link
Member

@matejmatuska matejmatuska left a comment

Choose a reason for hiding this comment

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

Just one small suggestion :) code-wise ok, I am going to test it now

repos/system_upgrade/common/libraries/repofileutils.py Outdated Show resolved Hide resolved
matejmatuska
matejmatuska requested changes Aug 2, 2024
View reviewed changes
Copy link
Member

@matejmatuska matejmatuska left a comment

Choose a reason for hiding this comment

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

The PR description is outdated, from my testing on RHEL 8 the output looks like this, which is good:

2024-08-02 12:17:22.052435 [ERROR] Actor: system_facts
Message: Invalid repository definition: copr:copr.fedorainfracloud.org:group_oamg:leapp in: /etc/yum.repos.d/leapp-copr.repo: The value of "name" field is None, but this is not allowed
Summary:
    Hint: For more directions on how to resolve the issue, see: https://access.redhat.com/solutions/6969001.
2024-08-02 12:17:33.302747 [ERROR] Actor: repositories_blacklist
Message: Actor didn't receive required messages: RepositoriesFacts

After the PR desc is updated, squash the commits and briefly describe the changes in the final one, otherwise LGTM.

DO NOT MERGE YET - FOR NOW IT'S NOT TARGETED FOR THE UPCOMING RELEASE

@pirat89 pirat89 added this to the 8.10/9.6 milestone Aug 6, 2024
@pirat89 pirat89 added the bug Something isn't working label Aug 7, 2024
@tomasfratrik
Copy link
Member Author

The latest push squashes commits.

@fernflower
Copy link
Member

Could you please add a unit test?

@matejmatuska
Copy link
Member

matejmatuska commented Aug 19, 2024 •
edited
Loading

Could you please add a unit test?

Agree, I don't know how I forgot that, sorry @tomasfratrik.

@tomasfratrik
Copy link
Member Author

Could you please add a unit test?

I added it!

matejmatuska
matejmatuska approved these changes Aug 29, 2024
View reviewed changes
Copy link
Member

@matejmatuska matejmatuska left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tests has been added and it works as expected. LGTM

STILL DO NOT MERGE

@tomasfratrik
Copy link
Member Author

The latest push squashed commits.

matejmatuska
matejmatuska requested changes Aug 29, 2024
View reviewed changes
Copy link
Member

@matejmatuska matejmatuska left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@pirat89 pointed out that a unit test was added, but only for the system_facts actor. Tests are still missing for the parse_repofile and potentially _parse_repository functions.

Also docstrings have to be included on public members in shared libraries (it would be okay in an actor lib).

repos/system_upgrade/common/libraries/repofileutils.py Outdated
Comment on lines 14 to 16
class InvalidRepoDefinition(Exception):
def __init__(self, msg, repofile, repoid):
Copy link
Member

Choose a reason for hiding this comment

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

Since this is public and in a shared library, add a docstring describing what is the exception used for.

Also update the docstring in the parse_repofile function to document that it can raise this exception and when.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tests were added and docstrings were updated.

@pirat89
Copy link
Member

pirat89 commented Sep 16, 2024

/packit copr-build

matejmatuska
matejmatuska requested changes Oct 25, 2024
View reviewed changes
Copy link
Member

@matejmatuska matejmatuska left a comment

Choose a reason for hiding this comment

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

I would add a little more test cases, otherwise ok.

repos/system_upgrade/common/libraries/tests/test_repofileutils.py Outdated Show resolved Hide resolved
@tomasfratrik
Copy link
Member Author

/packit copr-build

@tomasfratrik
Raise proper error when ModelViolationError occurs
ad4dbc3 
This error occurs when repo file has invalid definition, specifically
when the 'name' entry of the config files is invalid. Also add tests.

Jira: RHEL-19249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pirat89 pirat89 pirat89 left review comments

@matejmatuska matejmatuska matejmatuska requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
8.10/9.6
Development

Successfully merging this pull request may close these issues.
4 participants
@tomasfratrik @matejmatuska @fernflower @pirat89