chore: replace pylint and black with ruff #1382

Venefilyn · 2024-09-23T13:36:40Z

Ruff is a drop-in replacement for black and also handles linting
for us in a lot nicer way than pylint. More specifically it is a lot
faster than the alternatives and allows us to enforce PyFormat
for strings rather than percent-format or f-strings.

Jira Issues:

Checklist

PR has been tested manually in a VM (either author or reviewer)
Jira issue has been made public if possible
[RHELC-] or [HMS-] is part of the PR title
GitHub label has been added to help with Release notes
PR title explains the change from the user's point of view
Code and tests are documented properly
The commits are squashed to as few commits as possible (without losing data)
When merged: Jira issue has been updated to Release Pending if relevant

codecov · 2024-09-23T13:38:04Z

Codecov Report

Attention: Patch coverage is 95.86466% with 11 lines in your changes missing coverage. Please review.

Project coverage is 96.51%. Comparing base (27e14a4) to head (3a43a0b).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...ions/system_checks/check_firewalld_availability.py	50.00%	2 Missing ⚠️
convert2rhel/toolopts/config.py	75.00%	2 Missing ⚠️
convert2rhel/applock.py	75.00%	1 Missing ⚠️
convert2rhel/exceptions.py	0.00%	1 Missing ⚠️
convert2rhel/grub.py	94.44%	1 Missing ⚠️
convert2rhel/pkghandler.py	96.55%	1 Missing ⚠️
convert2rhel/redhatrelease.py	80.00%	1 Missing ⚠️
convert2rhel/subscription.py	94.11%	1 Missing ⚠️
convert2rhel/utils/__init__.py	96.42%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1382   +/-   ##
=======================================
  Coverage   96.51%   96.51%           
=======================================
  Files          71       71           
  Lines        5077     5077           
  Branches      883      883           
=======================================
  Hits         4900     4900           
  Misses         98       98           
  Partials       79       79

Flag	Coverage Δ
centos-linux-7	`92.01% <89.47%> (ø)`
centos-linux-8	`92.88% <87.59%> (ø)`
centos-linux-9	`92.92% <87.59%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Venefilyn · 2024-09-23T13:59:55Z

/packit test --labels sanity

tests/integration/tier0/non-destructive/config-file/test_config_file.py

hosekadam · 2024-09-24T13:14:16Z

pyproject.toml

+
+# We technically only support py27 py36 due to el7 and el8 but ruff only
+# supports py37 as minimum. This is not a huge issue as any rules we do not
+# want we can disablew


Suggested change

# want we can disablew

# want we can disable

Thanks, fixed

has-bot · 2024-09-25T07:28:07Z

This PR does not require integration tests to be run.

Comment generated by an automation.

Log | Bot Usage

has-bot · 2024-09-26T18:45:24Z

/packit test --labels sanity

Comment generated by an automation.

Log | Bot Usage

r0x0d · 2024-09-26T18:45:35Z

I've added the sanity test label just to be sure that nothing is broken. It will not, but just in case. Doesn't hurt to run the sanity tests suite

kokesak · 2024-09-27T06:57:10Z

/packit test --labels sanity

Venefilyn · 2024-09-27T13:36:30Z

/packit test --labels sanity

kokesak · 2024-09-30T07:07:21Z

tests/integration/common/checks-after-conversion/test_verify_strings_in_log.py

-from test_helpers.common_functions import log_file_data
-


Do not remove this line. It is needed in the test

Couldn't we just move it to conftest to have it be available without import? It doesn't feel good to have to import a fixture like this as it is a bit confusing. I'd rather have pytest take care of it in something like conftest

diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index bfb0596b..0734bf56 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -538,3 +538,20 @@ def backup_directory(shell, request): yield backup_path shell(f"rm -rf {backup_path}") + + +@pytest.fixture() +def log_file_data(): + """ + Helper fixture. + Reads and returns data from the convert2rhel.log file. + Mainly used for after conversion checks, where we match required strings to the log output. + """ + convert2rhel_log_file = "/var/log/convert2rhel/convert2rhel.log" + + assert os.path.exists(convert2rhel_log_file), "The c2r log file does not exits." + + with open(convert2rhel_log_file, "r") as logfile: + log_data = logfile.read() + + return log_data diff --git a/tests/integration/test_helpers/common_functions.py b/tests/integration/test_helpers/common_functions.py index 069626b1..c2cd9e07 100644 --- a/tests/integration/test_helpers/common_functions.py +++ b/tests/integration/test_helpers/common_functions.py @@ -108,20 +108,3 @@ def get_custom_repos_names(): f"rhel-{system_version.major}-for-x86_64-appstream-rpms", ] return repos - - -@pytest.fixture() -def log_file_data(): - """ - Helper fixture. - Reads and returns data from the convert2rhel.log file. - Mainly used for after conversion checks, where we match required strings to the log output. - """ - convert2rhel_log_file = "/var/log/convert2rhel/convert2rhel.log" - - assert os.path.exists(convert2rhel_log_file), "The c2r log file does not exits." - - with open(convert2rhel_log_file, "r") as logfile: - log_data = logfile.read() - - return log_data

We've moved certain functions to a separate file, so the conftest.py is not that big. It had over 1000 lines, and it was hard for us to navigate in it. The fixture log_file_data is used only in this test, so we moved it from conftest.

Venefilyn · 2024-09-30T13:34:45Z

/packit test --labels sanity

Venefilyn · 2024-10-01T11:01:14Z

/packit test --labels sanity

Venefilyn · 2024-10-01T13:59:59Z

/packit test --labels sanity

Venefilyn · 2024-10-01T15:59:17Z

@kokesak All integration tests are passing now with the change I made

# We use pytest_plugins to allow us to use fixtures we define in other files without the need to explicitly import them
# inside each test file.
# LINK - https://docs.pytest.org/en/7.0.x/reference/reference.html#globalvar-pytest_plugins
pytest_plugins = (
    "test_helpers.common_functions",
    "test_helpers.workarounds",
)

As for unit tests failing, it is unclear to me at the moment what the underlying issue is with the encoding tests in el7

But for the failing test relating to restorable file it is unrelated to any change I've made. It is just a unit test previously not being executed due to wrong name. Unsure if the error is the test or implementation atm

@r0x0d you are a code owner as well atm, wanna give a quick review of the devcontainer stuff that's changed?

kokesak

QE ack, thanks @Venefilyn!

Ruff is a drop-in replacement for black and also handles linting for us in a lot nicer way than pylint. More specifically it is a lot faster than the alternatives and allows us to enforce PyFormat for strings rather than percent-format or f-strings. Signed-off-by: Freya Gustavsson <[email protected]>

Take care of all the errors reported by Ruff with the current rules that are in place. This also takes care of percent-string formatting and replaces it with PyFormat. Signed-off-by: Freya Gustavsson <[email protected]> Co-authored-by: Rodolfo Olivieri <[email protected]>

Venefilyn · 2024-10-02T15:17:23Z

/packit test --labels sanity

Venefilyn · 2024-10-02T16:16:17Z

/packit test --labels sanity

Venefilyn requested review from r0x0d and a team as code owners September 23, 2024 13:36

Venefilyn requested a review from kokesak September 23, 2024 13:36

Venefilyn force-pushed the chore/pylint-format branch from 9f48ed7 to 7baec78 Compare September 23, 2024 13:52

kokesak reviewed Sep 24, 2024

View reviewed changes

tests/integration/tier0/non-destructive/config-file/test_config_file.py Outdated Show resolved Hide resolved

hosekadam reviewed Sep 24, 2024

View reviewed changes

Venefilyn force-pushed the chore/pylint-format branch 7 times, most recently from 3b13167 to b29ecad Compare September 24, 2024 17:51

kokesak added the tests-skip This PR does not require integration tests to be run. label Sep 25, 2024

kokesak approved these changes Sep 25, 2024

View reviewed changes

Venefilyn added the no-changelog If it should be excluded from changelog or Release notes. Such as infra, reverted PRs, etc. label Sep 25, 2024

hosekadam approved these changes Sep 25, 2024

View reviewed changes

r0x0d added tests-run-sanity PR ready to run the sanity test suit. Equivalent to `/packit test --labels sanity`. and removed tests-skip This PR does not require integration tests to be run. labels Sep 26, 2024

r0x0d force-pushed the chore/pylint-format branch from b29ecad to 5a6aff7 Compare September 26, 2024 18:51

Venefilyn force-pushed the chore/pylint-format branch from 5a6aff7 to 4aac204 Compare September 27, 2024 12:59

kokesak reviewed Sep 30, 2024

View reviewed changes

Venefilyn force-pushed the chore/pylint-format branch from d8b00da to 2547db4 Compare September 30, 2024 13:20

Venefilyn force-pushed the chore/pylint-format branch from 822e3a8 to 9619cb7 Compare October 1, 2024 12:04

kokesak approved these changes Oct 2, 2024

View reviewed changes

Venefilyn and others added 7 commits October 2, 2024 17:05

Revert change of importing before registering test helper rewrites

ef7961e

Add pytest_plugins to integration tests

d186051

Added workarounds to conftest

748a4ff

fix: yum import being incorrect

323302a

tests: Fixed failing encoding tests

3a43a0b

Venefilyn force-pushed the chore/pylint-format branch from 1c76273 to 3a43a0b Compare October 2, 2024 15:08

r0x0d approved these changes Oct 2, 2024

View reviewed changes

Venefilyn merged commit a805f5b into oamg:main Oct 2, 2024
26 of 29 checks passed

Venefilyn deleted the chore/pylint-format branch October 2, 2024 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: replace pylint and black with ruff #1382

chore: replace pylint and black with ruff #1382

Venefilyn commented Sep 23, 2024

codecov bot commented Sep 23, 2024 •

edited

Loading

Venefilyn commented Sep 23, 2024

hosekadam Sep 24, 2024

Venefilyn Sep 24, 2024

has-bot commented Sep 25, 2024

has-bot commented Sep 26, 2024

r0x0d commented Sep 26, 2024

kokesak commented Sep 27, 2024

Venefilyn commented Sep 27, 2024

kokesak Sep 30, 2024

Venefilyn Sep 30, 2024 •

edited

Loading

kokesak Sep 30, 2024

Venefilyn commented Sep 30, 2024

Venefilyn commented Oct 1, 2024

Venefilyn commented Oct 1, 2024

Venefilyn commented Oct 1, 2024

kokesak left a comment •

edited

Loading

Venefilyn commented Oct 2, 2024

Venefilyn commented Oct 2, 2024

chore: replace pylint and black with ruff #1382

chore: replace pylint and black with ruff #1382

Conversation

Venefilyn commented Sep 23, 2024

codecov bot commented Sep 23, 2024 • edited Loading

Codecov Report

Venefilyn commented Sep 23, 2024

hosekadam Sep 24, 2024

Choose a reason for hiding this comment

Venefilyn Sep 24, 2024

Choose a reason for hiding this comment

has-bot commented Sep 25, 2024

has-bot commented Sep 26, 2024

r0x0d commented Sep 26, 2024

kokesak commented Sep 27, 2024

Venefilyn commented Sep 27, 2024

kokesak Sep 30, 2024

Choose a reason for hiding this comment

Venefilyn Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

kokesak Sep 30, 2024

Choose a reason for hiding this comment

Venefilyn commented Sep 30, 2024

Venefilyn commented Oct 1, 2024

Venefilyn commented Oct 1, 2024

Venefilyn commented Oct 1, 2024

kokesak left a comment • edited Loading

Choose a reason for hiding this comment

Venefilyn commented Oct 2, 2024

Venefilyn commented Oct 2, 2024

codecov bot commented Sep 23, 2024 •

edited

Loading

Venefilyn Sep 30, 2024 •

edited

Loading

kokesak left a comment •

edited

Loading