feat(db): Add db #6

merkata · 2024-07-23T11:30:04Z

Applicable spec:

Overview

Adds a db relation and handling.

Rationale

Juju Events Changes

Module Changes

Library Changes

Checklist

[x ] The charm style guide was applied
[x ] The contributing guide was applied
[x ] The changes are compliant with ISD054 - Managing Charm Complexity
[x ] The documentation is generated using src-docs
[x ] The documentation for charmhub is updated
[x ] The PR is tagged with appropriate label (urgent, trivial, complex)

github-actions

license-eye has checked 40 files.

Valid	Invalid	Ignored	Fixed
17	1	22	0

Click to see the invalid file list

tests/unit/test_irc.py

Use this command to fix any missing license headers

```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

tests/unit/test_irc.py

src/charm.py

src/database.py

amandahla · 2024-07-23T13:09:01Z

src/database.py

+            data["POSTGRES_PASSWORD"],
+            data["POSTGRES_DB"],
+        ):
+            return default


Will IRC bridge fail if the default is returned?

Same comment on my side as before

amandahla · 2024-07-23T13:12:57Z

src/database.py

+
+        return data
+
+    def is_relation_ready(self) -> bool:


This is interesting.
Most of our charms are relying on some kind of "database is not None" (at least is my impression).
The only concern here is that by any chance this check is not done before using the relation data, will be hard to tell why there is no database configuration as expected.

This should be managed through a ValidationError when the pydantic object is created more than an explicit is ready

src/irc.py

src/database.py

src/charm.py

src/irc.py

src/database.py

gregory-schiano · 2024-07-23T14:39:21Z

src/irc.py

+        exec_process = subprocess.run(
+           pem_create_command, shell=True, check=True, capture_output=True)
+        )


Any exception that we should catch here ?

src/irc.py

src/charm.py

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

license-eye has checked 45 files.

Valid	Invalid	Ignored	Fixed
19	3	23	0

Click to see the invalid file list

templates/config.yaml
templates/matrix-appservice-irc.service
templates/matrix-appservice-irc.target

Use this command to fix any missing license headers

```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

github-actions · 2024-07-24T14:20:11Z

config.yaml

@@ -0,0 +1,21 @@
+


Suggested change

# Copyright 2024 Canonical Ltd.

# See LICENSE file for licensing details.

templates/config.yaml

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

license-eye has checked 49 files.

Valid	Invalid	Ignored	Fixed
22	3	24	0

Click to see the invalid file list

templates/matrix-appservice-irc.service
templates/matrix-appservice-irc.target
tests/unit/test_charm_types.py

Use this command to fix any missing license headers

```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

github-actions · 2024-08-23T05:44:36Z

tests/unit/test_charm_types.py

@@ -0,0 +1,26 @@
+import pytest


Suggested change

import pytest

# Copyright 2024 Canonical Ltd.

# See LICENSE file for licensing details.

import pytest

gregory-schiano · 2024-09-20T12:39:08Z

metadata.yaml

You should merge charmcraft.yaml and metadata.yaml in charmcraft.yaml

I'm facing an issue here when merging:

Bad charmcraft.yaml content: - input should be None (in field 'issues') - extra inputs are not permitted (in field 'display-name') - extra inputs are not permitted (in field 'docs') - extra inputs are not permitted (in field 'maintainers') - extra inputs are not permitted (in field 'source')

Might leave it as is for now (stumbled upon this).

requirements.txt

src/charm.py

src/database_observer.py

src/irc.py

tests/integration/test_charm.py

src/charm.py

gregory-schiano · 2024-09-25T11:30:48Z

src/irc.py

+            + "-outform PEM -algorithm RSA -pkeyopt rsa_keygen_bits:2048",
+        ]
+        logger.info("Creating PEM file for IRC bridge.")
+        subprocess.run(pem_create_command, shell=True, check=True, capture_output=True)  # nosec


What did we say about that one ? I see the comment is closed but without being addressed

src/irc.py

gregory-schiano · 2024-09-25T11:37:13Z

src/irc.py

    """Exception raised when unable to stop the service."""


 class InstallError(exceptions.SnapError):
    """Exception raised when unable to install dependencies for the service."""


-class IRCBRidgeService:
+class IRCBridgeService:


There are not instance or class attributes nor constructor. I'm wondering if this shouldn't simply be a module ?

It could, that's in spirit of the bind-operator, it has a similar class (BindService) without a constructor or attributes. Propose to leave it as is for now.

Ok, but let's track the change in a ticket

gregory-schiano · 2024-09-25T11:40:27Z

src/irc.py

+            "-c",
+            f"[[ -f {IRC_BRIDGE_REGISTRATION_FILE_PATH} ]] || "
+            f"matrix-appservice-irc -r -f {IRC_BRIDGE_REGISTRATION_FILE_PATH}"
+            f" -u https://{matrix.host}:{IRC_BRIDGE_HEALTH_PORT} "


How do we intend to route traffic to the bridge machine ?
If they share the same host as matrix, then we need a reverse proxy in front of both that will route based on path or port since the hostname will resolve to the same IP for both synapse and the bridge

Well spotted, this is not correct, it should be the address where the Matrix homeserver can contact the bridge. It can be a config option if we use an FQDN or an IP that is reachable from Matrix, wdyt?

Yeap that's tricky... the machine needs either to be accessible publicly or internally, and it should support HTTPS meaning you need either a TLS termination or support the TLS relation in the bridge :\

Depends on the communication flow, I think Matrix and the bridge talk to each other, soo it could be private? And the public part between the IRC network and the bridge happens over the Mattix homeserver. Need to check, you raised a very good point here.

github-actions · 2024-09-25T15:19:06Z

Test coverage for 157d588

Name                       Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------
src/charm.py                  57     57      2      0     0%   7-110
src/charm_types.py            37      2      8      2    91%   102, 106
src/constants.py              20      0      0      0   100%
src/database_observer.py      23      2      2      1    88%   52, 63
src/exceptions.py             12      2      0      0    83%   52, 68
src/irc.py                    90      6     14      1    93%   140-142, 195->197, 202-204
src/matrix_observer.py        13     13      0      0     0%   6-42
----------------------------------------------------------------------
TOTAL                        252     82     26      4    68%

Static code analysis report

Run started:2024-09-25 13:27:01.084269

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1157
  Total lines skipped (#nosec): 7
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

merkata · 2024-10-01T07:39:37Z

Merged as discussed - follow up work in the feat/add-plugins-lib where the rest of the comments are addressed. The failing test is due to a snap issue that is waiting to be resolved via a manual approval in the snap store (usage of a privileged interface).

feat(db): Add draft db work

0924369

github-actions bot added the Libraries: OK label Jul 23, 2024

github-actions bot reviewed Jul 23, 2024

View reviewed changes

tests/unit/test_irc.py Outdated Show resolved Hide resolved