Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Rabbitmq integration plus integrations refactor #41

Closed
wants to merge 25 commits into from

Conversation

javierdelapuente
Copy link
Collaborator

@javierdelapuente javierdelapuente commented Sep 5, 2024

Applicable spec: ISD175 - RabbitMQ support in paas-charm

Overview

Support RabbitMQ in paas-app-charmer.

There are two different charms that provider RabbitMQ, rabbitmq-server and rabbitmq-k8s. A new library has done, based on the one in rabbitmq-k8s, paas_app_charmer.rabbitmq.RabbitMQRequires that provides a common interface to both charms, and provides a RabbitMQParameters model with the rabbitmq information. This is the same as the one in the charm state, as that library is just use in this project.

A refactor has been done, moving most of the code related to integrations from charm.py, databases.py and charm_state.py to charm_integrations.py. The integrations are managed now by the class paas_app_charmer.charm_integrations.Integrations, that is in charge of registering all integrations, checking if there are missing integrations and also in creating the IntegrationsState instance (part of the CharmState).

Each one of the integrations are subclasses of Integration, so they all contain register and is_ready methods, and are handled in a similar way. Each of these integrations can create its own part of the charm state for the integration (before this code was in charm_state.py). This code may require further improvements in future PRs.

Rationale

Juju Events Changes

Module Changes

Library Changes

Checklist

@javierdelapuente javierdelapuente changed the title WIP Rabbitmq integrations plus refactor Rabbitmq integration plus integrations refactor Sep 5, 2024
Copy link

github-actions bot commented Sep 5, 2024

Test coverage for b8fe43c

Name                                            Stmts   Miss Branch BrPart  Cover   Missing
-------------------------------------------------------------------------------------------
paas_app_charmer/__init__.py                       39     18      0      0    54%   17-18, 23-24, 30-31, 37-38, 44-45, 51-52, 58-59, 66-67, 72-73
paas_app_charmer/_gunicorn/__init__.py              0      0      0      0   100%
paas_app_charmer/_gunicorn/charm.py                15      0      0      0   100%
paas_app_charmer/_gunicorn/webserver.py            83      4     16      1    95%   175, 187-193
paas_app_charmer/_gunicorn/workload_config.py       8      0      0      0   100%
paas_app_charmer/_gunicorn/wsgi_app.py             16      0      0      0   100%
paas_app_charmer/app.py                           126      1     52      3    98%   95->exit, 145->151, 289
paas_app_charmer/charm.py                         133     14     18      4    87%   140-141, 143-144, 165->exit, 177-181, 205-207, 253-254, 259, 264
paas_app_charmer/charm_integrations.py            173      7     39      2    96%   245, 273-274, 359-360, 494-495
paas_app_charmer/charm_state.py                    87      1     10      1    98%   166
paas_app_charmer/charm_utils.py                    23      0      0      0   100%
paas_app_charmer/database_migration.py             35      0      2      0   100%
paas_app_charmer/django/__init__.py                 2      0      0      0   100%
paas_app_charmer/django/charm.py                   34      4      6      2    85%   44, 81, 96-97
paas_app_charmer/exceptions.py                      5      0      0      0   100%
paas_app_charmer/fastapi/__init__.py                2      0      0      0   100%
paas_app_charmer/fastapi/charm.py                  29      0      0      0   100%
paas_app_charmer/flask/__init__.py                  2      0      0      0   100%
paas_app_charmer/flask/charm.py                    24      0      0      0   100%
paas_app_charmer/go/__init__.py                     2      0      0      0   100%
paas_app_charmer/go/charm.py                       26      0      0      0   100%
paas_app_charmer/observability.py                  20      0      6      1    96%   41->45
paas_app_charmer/rabbitmq.py                       76      1     24      4    95%   116->exit, 151->exit, 156, 175->169
paas_app_charmer/secret_storage.py                 50      3     16      5    88%   51, 55->54, 56->58, 86, 105
paas_app_charmer/utils.py                          21      4     14      2    71%   30, 32, 52-53
-------------------------------------------------------------------------------------------
TOTAL                                            1031     57    203     25    93%

Static code analysis report

Run started:2024-09-05 13:04:12.743396

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 2284
  Total lines skipped (#nosec): 1
  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):

@@ -115,6 +115,11 @@ requires:
interface: saml
optional: True
limit: 1
amqp:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it have to be called this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will update to rabbitmq in the other pr. The reason for that was because that was the name in rabbitmq-server (and amqp is the protocol)

import charms.data_platform_libs.v0.s3 # noqa: F401
except ImportError:
logger.exception(
"Missing charm library, please run `charmcraft fetch-lib charms.data_platform_libs.v0.s3`"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we just say run fetch-libs?

Comment on lines +64 to +65
self.charm_integrations = Integrations(self)
self.charm_integrations.register(self._on_generic_integration_event)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I like it, I propose we go with the if-else ladder for now and get advice from @tonyandrewmeyer from the ops team on how to reduce it

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't do this because it takes away the option of the charm developer customising the code. All the observe need to be here and all the functions need to be defined on the class so that an adventurous Flask developer can change how they work and still be able to use the machinery we have built

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, we can go without the refactor (I have it in the other PR).

However, I do not see why that would limit a user, everything can be overridden as it was before...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(talked outside here) the reason is that there is only one callback function for all of the integrations

Choose a reason for hiding this comment

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

It sounds like this is resolved now, but let me know if you'd still like input on anything :)

@javierdelapuente
Copy link
Collaborator Author

closing the PR for now.

@javierdelapuente javierdelapuente mentioned this pull request Sep 6, 2024
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants