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

feature(pre-commit): replace pylint with ruff #5799

Merged
merged 2 commits into from
Jun 17, 2024
Merged

feature(pre-commit): replace pylint with ruff #5799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d05b206
feature(pre-commit): replace pylint with ruff #5799
fruch Dec 14, 2023
ecd3240
feature(ruff): add blind except (BLE) checks
fruch Dec 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,10 @@ repos:
types: [python]
exclude: '\.sh$'

- id: pylint
name: pylint
entry: pylint -j 2 -d consider-using-f-string
- id: ruff
name: ruff
entry: ruff check --fix
language: system
exclude: ^docker/alternator-dns/.*$
types: [python]

- repo: https://github.com/alessandrojcm/commitlint-pre-commit-hook
Expand Down
2 changes: 1 addition & 1 deletion artifacts_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ def get_email_data(self):
email_data = self._get_common_email_data()
try:
node = self.node
except Exception: # pylint: disable=broad-except
except (ValueError, IndexError):
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be also AttributeError and some other.
I don't see any benefit narrowing down the exception coverage here.

Copy link
Contributor Author

@fruch fruch Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

since this is the implementation, those are the only two options:

    @property
    def node(self):
        if self.db_cluster is None or not self.db_cluster.nodes:
            raise ValueError('DB cluster has not been initiated')
        return self.db_cluster.nodes[0]

Copy link
Contributor

Choose a reason for hiding this comment

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

The AttributeError is totally possible here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, if you remove db_cluster assignment from Tester.__init__

Copy link
Contributor

Choose a reason for hiding this comment

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

The absence of the nodes attr is also one of possible causes.
So, the statement those are the only two options is false because there are 3 different possible exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as before it's in nodes are being assigned in BaseCluster.__init__

to sum it up, we know exactly what can get broken here, i.e. the list of node can be empty, cause test failed before starting the node.

node = None
if node:
scylla_packages = node.scylla_packages_installed
Expand Down
6 changes: 3 additions & 3 deletions docker/alternator-dns/dns_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@


def livenodes_update():
global alternator_port
global livenodes
global alternator_port # noqa: PLW0602
global livenodes # noqa: PLW0603
while True:
# Contact one of the already known nodes by random, to fetch a new
# list of known nodes.
Expand All @@ -34,7 +34,7 @@ def livenodes_update():
# If we're successful, replace livenodes by the new list
livenodes = a
print(livenodes)
except Exception: # pylint: disable=broad-except
except Exception: # pylint: disable=broad-except # noqa: BLE001
fruch marked this conversation as resolved.
Show resolved Hide resolved
# TODO: contacting this ip was unsuccessful, maybe we should
# remove it from the list of live nodes.
pass
Expand Down
2 changes: 1 addition & 1 deletion docker/env/version
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1.69-update-scylla-driver-3.26.8
1.70-introduce-ruff
4 changes: 2 additions & 2 deletions functional_tests/scylla_operator/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def publish_test_result():

@pytest.fixture(autouse=True, scope='package', name="tester")
def fixture_tester() -> ScyllaOperatorFunctionalClusterTester:
global TESTER # pylint: disable=global-statement
global TESTER # pylint: disable=global-statement # noqa: PLW0603
fruch marked this conversation as resolved.
Show resolved Hide resolved
os.chdir(sct_abs_path())
tester_inst = ScyllaOperatorFunctionalClusterTester()
TESTER = tester_inst # putting tester global, so we can report skipped test (one with mark.skip)
Expand Down Expand Up @@ -175,7 +175,7 @@ def _bring_cluster_back_to_original_state(
db_cluster.restart_scylla()
db_cluster.wait_for_nodes_up_and_normal(
nodes=db_cluster.nodes, verification_node=db_cluster.nodes[0])
except Exception as exc: # pylint: disable=broad-except
except Exception as exc: # pylint: disable=broad-except # noqa: BLE001
fruch marked this conversation as resolved.
Show resolved Hide resolved
tester.healthy_flag = False
pytest.fail("Failed to bring cluster nodes back to original state due to :\n" +
"".join(traceback.format_exception(type(exc), exc, exc.__traceback__)))
Expand Down
4 changes: 2 additions & 2 deletions functional_tests/scylla_operator/test_functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ def wait_for_cleanup_logs(log_follower_name, log_follower, db_cluster):
time.sleep(4)
db_cluster.nodes[0].run_cqlsh(cmd=f"DROP KEYSPACE IF EXISTS {current_ks_name}", timeout=60)
time.sleep(4)
except Exception as exc: # pylint: disable=broad-except
except Exception as exc: # pylint: disable=broad-except # noqa: BLE001
fruch marked this conversation as resolved.
Show resolved Hide resolved
# NOTE: we don't care if some of the queries fail.
# At first, there are redundant ones and, at second, they are utilitary.
log.warning("Utilitary CQL query has failed: %s", exc)
Expand Down Expand Up @@ -646,7 +646,7 @@ def change_cluster_spec() -> None:
# NOTE: increase the value only when the sysctl spec update is successful
# to avoid false negative results in further assertions
expected_aio_max_nr_value += 1
except Exception as error: # pylint: disable=broad-except
except Exception as error: # pylint: disable=broad-except # noqa: BLE001
str_error = str(error)
log.debug("Change /spec/sysctls value to %d failed. Error: %s",
expected_aio_max_nr_value, str_error)
Expand Down
8 changes: 2 additions & 6 deletions longevity_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ def chunks(_list, chunk_size):
self._pre_create_templated_user_schema(batch_start=extra_tables_idx,
batch_end=extra_tables_idx+num_of_newly_created_tables)
for i in range(num_of_newly_created_tables):
batch += self.create_templated_user_stress_params(extra_tables_idx + i, cs_profile=cs_profile)
batch.append(self.create_templated_user_stress_params(extra_tables_idx + i, cs_profile=cs_profile))

nodes_ips = self.all_node_ips_for_stress_command
for params in batch:
Expand Down Expand Up @@ -464,13 +464,9 @@ def _flush_all_nodes(self):

def get_email_data(self):
self.log.info("Prepare data for email")
email_data = {}
grafana_dataset = {}

try:
email_data = self._get_common_email_data()
except Exception as error: # pylint: disable=broad-except
self.log.exception("Error in gathering common email data: Error:\n%s", error, exc_info=error)
email_data = self._get_common_email_data()
vponomaryov marked this conversation as resolved.
Show resolved Hide resolved

try:
grafana_dataset = self.monitors.get_grafana_screenshot_and_snapshot(
Expand Down
1 change: 1 addition & 0 deletions mgmt_cli_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,7 @@ def test_repair_multiple_keyspace_types(self): # pylint: disable=invalid-name
keyspace_repair_percentage = per_keyspace_progress.get(keyspace_name, None)
assert keyspace_repair_percentage is not None, \
"The keyspace {} was not included in the repair!".format(keyspace_name)

assert keyspace_repair_percentage == 100, \
"The repair of the keyspace {} stopped at {}%".format(
keyspace_name, keyspace_repair_percentage)
Expand Down
5 changes: 2 additions & 3 deletions mgmt_upgrade_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,8 @@ def validate_previous_task_details(task, previous_task_details):
# and as a result it could be a BIT imprecise
if abs(delta.total_seconds()) > 60:
mismatched_details_name_list.append(detail_name)
else:
if current_value != previous_task_details[detail_name]:
mismatched_details_name_list.append(detail_name)
elif current_value != previous_task_details[detail_name]:
mismatched_details_name_list.append(detail_name)
complete_error_description = _create_mismatched_details_error_message(previous_task_details,
current_task_details,
mismatched_details_name_list)
Expand Down
2 changes: 1 addition & 1 deletion performance_regression_row_level_repair_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def preload_data(self, consistency_level=None):

for stress_cmd in prepare_write_cmd:
if consistency_level:
stress_cmd = self._update_cl_in_stress_cmd(
stress_cmd = self._update_cl_in_stress_cmd( # noqa: PLW2901
str_stress_cmd=stress_cmd, consistency_level=consistency_level)
params.update({'stress_cmd': stress_cmd})

Expand Down
2 changes: 1 addition & 1 deletion performance_regression_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ def display_results(self, results, test_name=''):
with open(os.path.join(self.logdir, 'jenkins_perf_PerfPublisher.xml'), 'w', encoding="utf-8") as pref_file:
content = """<report name="%s report" categ="none">%s</report>""" % (test_name, test_xml)
pref_file.write(content)
except Exception as ex: # pylint: disable=broad-except
except Exception as ex: # pylint: disable=broad-except # noqa: BLE001
fruch marked this conversation as resolved.
Show resolved Hide resolved
self.log.debug('Failed to display results: {0}'.format(results))
self.log.debug('Exception: {0}'.format(ex))

Expand Down
16 changes: 16 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[tool.ruff]
lint.select = ["PL", "YTT", "BLE"]

lint.ignore = ["E501", "PLR2004"]

target-version = "py310"

force-exclude = true
line-length = 240
fruch marked this conversation as resolved.
Show resolved Hide resolved
respect-gitignore = true


[tool.ruff.lint.pylint]
max-args = 12
max-statements = 100
max-branches = 24
2 changes: 1 addition & 1 deletion requirements.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ python-jenkins==1.7.0
ssh2-python==1.0.0
argus-alm==0.12.3
parameterized==0.8.1
pylint==2.11.1 # Needed for pre-commit hooks
ruff==0.4.7 # Needed for pre-commit hooks
autopep8==1.5.7 # Needed for pre-commit hooks
kubernetes==24.2.0
packaging==21.3
Expand Down
Loading