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

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Fix ModelViolationError while parsing repo files #1266

merged 1 commit into from
Nov 1, 2024

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 force-pushed the fix-modelviolation-error branch from 2a0f448 to 3bd08f8 Compare July 19, 2024 13:52
@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

@tomasfratrik tomasfratrik force-pushed the fix-modelviolation-error branch from 3bd08f8 to 49708b5 Compare August 6, 2024 08:38
@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 tomasfratrik force-pushed the fix-modelviolation-error branch from 49708b5 to 9ed7217 Compare August 15, 2024 07:30
@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 tomasfratrik force-pushed the fix-modelviolation-error branch from 9ed7217 to 880fe49 Compare August 28, 2024 14:03
@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 tomasfratrik force-pushed the fix-modelviolation-error branch from 880fe49 to 94ffbb8 Compare August 29, 2024 11:19
@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 Show resolved Hide resolved
@tomasfratrik tomasfratrik force-pushed the fix-modelviolation-error branch from 94ffbb8 to 5a584b8 Compare September 3, 2024 14:45
@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 tomasfratrik force-pushed the fix-modelviolation-error branch from 5a584b8 to 0a55672 Compare October 28, 2024 22:23
@tomasfratrik
Copy link
Member Author

/packit copr-build

@tomasfratrik tomasfratrik force-pushed the fix-modelviolation-error branch from 0a55672 to 3af8eec Compare October 29, 2024 13:37
@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
@tomasfratrik tomasfratrik force-pushed the fix-modelviolation-error branch from 3af8eec to ad4dbc3 Compare October 30, 2024 13:10
matejmatuska
matejmatuska approved these changes Oct 31, 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.

Integration tests on RHEL7 are broken everywhere, not cause by these changes.

LGTM

@pirat89
Copy link
Member

pirat89 commented Nov 1, 2024

/packit copr-build

@pirat89
Copy link
Member

pirat89 commented Nov 1, 2024

@matejmatuska seems that the non-els error we discussed has been caused just by infra issue. merging.

@pirat89 pirat89 merged commit ec07824 into oamg:main Nov 1, 2024
22 of 26 checks passed
@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Nov 1, 2024
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 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant
Projects
None yet
Milestone
8.10/9.6
Development

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