[MISC] Add linters #667

dragomirp · 2025-08-07T13:15:50Z

Adding new linters:

Checklist

I have added or updated any relevant documentation.
I have cleaned any remaining cloud resources from my accounts.

dragomirp · 2025-08-07T13:56:48Z

lib/charms/mysql/v0/mysql.py

+        # TODO Should we be using md5 here?
+        return hashlib.md5(random_characters.encode("utf-8")).hexdigest()  # noqa: S324


Shouldn't be a security problem, but it might be better to use a different algorithm or just use the random chars of the expected length.

s/md5/sha256/

It could be worth to put algorithm changes into the separate followup PR (to avoid hiding it into Linting changes).

I use shake_128 in such cases to keep the value short without being bothered by security linters:

>>> stri= b"test" >>> hashlib.md5(stri).hexdigest() '098f6bcd4621d373cade4e832627b4f6' >>> hashlib.shake_128(stri).hexdigest(16) 'd3b0aa9cd8b7255622cebc631e867d40' >>> hashlib.sha256(stri).hexdigest() '9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08'

But either way, best to be done separately.

lib/charms/mysql/v0/mysql.py

dragomirp · 2025-08-07T14:52:58Z

lib/pyproject.toml

+[tool.ruff]
+# preview and explicit preview are enabled for CPY001
+preview = true
+target-version = "py38"


Focal compatibility for the libs.

dragomirp · 2025-08-07T14:53:13Z

pyproject.toml


 [tool.poetry.group.format.dependencies]
-ruff = "^0.4.5"
+ruff = "^0.12.7"


Bump to latest ruff.

Renovate config to bump it further?

It's in PR #425

dragomirp · 2025-08-07T14:53:41Z

pyproject.toml

 # preview and explicit preview are enabled for CPY001
 preview = true
-target-version = "py38"
+target-version = "py310"


Use jammy python for non-libs.

dragomirp · 2025-08-07T15:42:32Z

src/charm.py

+                    # TODO use set_ports instead
+                    subprocess.check_call(["open-port", f"{port}/tcp"])  # noqa: S603 S607


Ops set_ports should be able to do the same thing without having to call external utilities.

indeed. my guess this bit predates set_ports

dragomirp · 2025-08-07T15:45:50Z

lib/charms/mysql/v0/mysql.py

-            if skip_release_lock:
-                return


Linting rule

hmmm. Ok that makes sense

dragomirp · 2025-08-07T15:47:03Z

tests/integration/test_backup_aws.py

+            # Awaitied insed the loop
+            lambda: unit.workload_status == "active",  # noqa: B023


Doesn't seem dangerous, but should the test await serially here?

yeah, there's room for improvements. Jubilant migration will be the time

dragomirp · 2025-08-07T15:49:14Z

tests/integration/helpers.py

+    except RetryError as e:
+        raise Exception("Failed to start server.") from e


IMHO it's better to enable reraise in Retrying and remove the custom exception, since it makes it easier to figure out what went wrong.

on charm code yes. On test either way. Fell free

paulomach

GH is not showing big diffs. Will resume the review tomorrow.

paulomach · 2025-08-07T21:24:33Z

lib/charms/mysql/v0/backups.py

+from ops.charm import ActionEvent
+from ops.framework import Object
+from ops.jujuversion import JujuVersion
+from ops.model import BlockedStatus, MaintenanceStatus


paulomach · 2025-08-07T21:25:42Z

lib/charms/mysql/v0/backups.py

            f"Application Name: {self.model.app.name}\n"
            f"Unit Name: {self.charm.unit.name}\n"
-            f"Juju Version: {str(juju_version)}\n"
+            f"Juju Version: {juju_version!s}\n"


paulomach · 2025-08-07T21:28:25Z

lib/pyproject.toml

+select = ["A", "E", "W", "F", "C", "N", "D", "I", "B", "CPY001", "RUF", "S", "SIM", "UP", "TC"]
+ignore = [
+  "D107", # Ignore D107 Missing docstring in __init__
+  "E501", # Ignore E501 Line too long


are we ignoring long lines on PG also? I would prefer not to ignore it.

@paulomach you set LGTM, should this be addressed before merging or followup?

The deep lore is that it comes from the charmcraft templates. I guess it was set for compatibility with black, which was the default formatter before ruff started doing that as well.

Shouldn't practically matter, since the subsequent formatting check will complain about it, but most probably should be removed as legacy fluff.

Tried to disable here and in the main pyproject, but causes additional violations with long strings. I guess that the formatter can't handle cutting the strings down.

Merging as is for the time being.

paulomach · 2025-08-07T21:33:33Z

src/charm.py

+                    # TODO use set_ports instead
+                    subprocess.check_call(["open-port", f"{port}/tcp"])  # noqa: S603 S607


indeed. my guess this bit predates set_ports

paulomach · 2025-08-07T21:40:56Z

src/ip_address_observer.py


-        if pid := self.charm.unit_peer_data.get("observer-pid"):
-            if check_pid(int(pid)):
-                return


that's embarrassing

paulomach · 2025-08-07T21:57:52Z

tests/integration/helpers.py

+    except RetryError as e:
+        raise Exception("Failed to start server.") from e


on charm code yes. On test either way. Fell free

paulomach

worked. Drago, my hero

paulomach · 2025-08-07T22:06:21Z

lib/charms/mysql/v0/mysql.py

+        # TODO Should we be using md5 here?
+        return hashlib.md5(random_characters.encode("utf-8")).hexdigest()  # noqa: S324


s/md5/sha256/

lib/charms/mysql/v0/mysql.py

paulomach · 2025-08-07T22:14:01Z

lib/charms/mysql/v0/mysql.py

-            if skip_release_lock:
-                return


hmmm. Ok that makes sense

paulomach · 2025-08-07T22:23:28Z

tests/integration/test_backup_aws.py

+            # Awaitied insed the loop
+            lambda: unit.workload_status == "active",  # noqa: B023


yeah, there's room for improvements. Jubilant migration will be the time

taurus-forever

Well.... tiny PR to enable Lints they said. WELL DONE!

taurus-forever · 2025-08-08T08:51:30Z

lib/charms/mysql/v0/mysql.py

+        # TODO Should we be using md5 here?
+        return hashlib.md5(random_characters.encode("utf-8")).hexdigest()  # noqa: S324


It could be worth to put algorithm changes into the separate followup PR (to avoid hiding it into Linting changes).

lib/charms/mysql/v0/mysql.py

taurus-forever · 2025-08-08T08:56:10Z

lib/pyproject.toml

+select = ["A", "E", "W", "F", "C", "N", "D", "I", "B", "CPY001", "RUF", "S", "SIM", "UP", "TC"]
+ignore = [
+  "D107", # Ignore D107 Missing docstring in __init__
+  "E501", # Ignore E501 Line too long


@paulomach you set LGTM, should this be addressed before merging or followup?

taurus-forever · 2025-08-08T08:56:22Z

poetry.lock

@@ -1,4 +1,4 @@
-# This file is automatically @generated by Poetry 2.1.3 and should not be changed by hand.
+# This file is automatically @generated by Poetry 2.1.2 and should not be changed by hand.


Because it's my locally installed Poetry version and it sets the version of poetry used to generate the lock.

taurus-forever · 2025-08-08T08:56:54Z

pyproject.toml


 [tool.poetry.group.format.dependencies]
-ruff = "^0.4.5"
+ruff = "^0.12.7"


Renovate config to bump it further?

dragomirp added 3 commits August 7, 2025 13:36

Autolint

39f0e42

Manual changes

7cf3512

Final format

3a7bbc7

dragomirp added the not bug or enhancement PR is not 'bug' or 'enhancement'. For release notes label Aug 7, 2025

github-actions bot added the Libraries: Out of sync label Aug 7, 2025

dragomirp commented Aug 7, 2025

View reviewed changes

lib/charms/mysql/v0/mysql.py Show resolved Hide resolved

dragomirp commented Aug 7, 2025

View reviewed changes

dragomirp added 2 commits August 7, 2025 17:54

Typo

61ea8bc

Bugbears

6ddf34c

dragomirp force-pushed the add-linters branch from 937486d to 6ddf34c Compare August 7, 2025 15:40

dragomirp commented Aug 7, 2025

View reviewed changes

dragomirp marked this pull request as ready for review August 7, 2025 18:37

dragomirp requested review from a team, carlcsaposs-canonical, paulomach, shayancanonical, sinclert-canonical and taurus-forever and removed request for a team August 7, 2025 18:37

paulomach reviewed Aug 7, 2025

View reviewed changes

paulomach approved these changes Aug 7, 2025

View reviewed changes

taurus-forever approved these changes Aug 8, 2025

View reviewed changes

dragomirp merged commit 57441ed into main Aug 8, 2025
450 of 455 checks passed

dragomirp deleted the add-linters branch August 8, 2025 12:39

		# TODO Should we be using md5 here?
		return hashlib.md5(random_characters.encode("utf-8")).hexdigest() # noqa: S324

		# TODO use set_ports instead
		subprocess.check_call(["open-port", f"{port}/tcp"]) # noqa: S603 S607

		# Awaitied insed the loop
		lambda: unit.workload_status == "active", # noqa: B023

		except RetryError as e:
		raise Exception("Failed to start server.") from e

		@@ -1,4 +1,4 @@
		# This file is automatically @generated by Poetry 2.1.3 and should not be changed by hand.
		# This file is automatically @generated by Poetry 2.1.2 and should not be changed by hand.

[MISC] Add linters #667

[MISC] Add linters #667

Uh oh!

Conversation

dragomirp commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragomirp Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragomirp Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulomach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulomach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taurus-forever left a comment

Choose a reason for hiding this comment

dragomirp commented Aug 7, 2025 •

edited

Loading

dragomirp Aug 7, 2025 •

edited

Loading

dragomirp Aug 8, 2025 •

edited

Loading