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

Move the content of the flask-k8s charm to a separate Python library #70

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Move the content of the flask-k8s charm to a separate Python library #70

wants to merge 5 commits into from

Conversation

weiiwang01
Copy link
Collaborator

Overview

Refactor the flask-k8s charm by transferring the majority of its logic to the xiilib Python library.

Rationale

By creating a universal library for all 12-factor charms, we enable the reuse of components across various 12-factor charms. Additionally, this approach reduces the flask-k8s charm's size, making it fits into a template.

Checklist

@weiiwang01 weiiwang01 marked this pull request as ready for review November 10, 2023 14:35
@weiiwang01 weiiwang01 requested a review from a team as a code owner November 10, 2023 14:35
yanksyoon
yanksyoon previously approved these changes Nov 11, 2023
View reviewed changes
Copy link

@yanksyoon yanksyoon left a comment

Choose a reason for hiding this comment

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

Lgtm!

.gitignore Outdated
@@ -8,3 +8,5 @@ __pycache__/
.idea
.vscode/
*.rock
*.egg-info/
dist/
Copy link

Choose a reason for hiding this comment

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

I think there is a missing newline here :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added, thanks!

tox.ini Show resolved Hide resolved
arturo-seijas
arturo-seijas reviewed Nov 13, 2023
View reviewed changes
Copy link
Collaborator

@arturo-seijas arturo-seijas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You are also changing the ingress lib version as part of this PR. Can you update the description? Also, it would be better next time to have changes as atomic as possible in each PR for easier review, among other reasons.
I haven't seen a PR for review for the new repository. Can you link it here?

arturo-seijas
arturo-seijas reviewed Nov 13, 2023
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
@weiiwang01 weiiwang01 mentioned this pull request Nov 15, 2023
Closed
6 tasks
@weiiwang01
Copy link
Collaborator Author

You are also changing the ingress lib version as part of this PR. Can you update the description? Also, it would be better next time to have changes as atomic as possible in each PR for easier review, among other reasons. I haven't seen a PR for review for the new repository. Can you link it here?

Sorry I forgot there's already an old pull request to update the ingress charm library #68. I will merge that shortly after the CI passed.

For the new repository, it's here https://github.com/canonical/xiilib. But there's not much thing we can do in that repository (CI/CD) since that's highly dependent on the charmcraft update (new init profile). For now, I just pined the commit version of the xiilib library, but I will update that repository to make it more formal.

@arturo-seijas
Copy link
Collaborator

You are also changing the ingress lib version as part of this PR. Can you update the description? Also, it would be better next time to have changes as atomic as possible in each PR for easier review, among other reasons. I haven't seen a PR for review for the new repository. Can you link it here?

Sorry I forgot there's already an old pull request to update the ingress charm library #68. I will merge that shortly after the CI passed.

For the new repository, it's here https://github.com/canonical/xiilib. But there's not much thing we can do in that repository (CI/CD) since that's highly dependent on the charmcraft update (new init profile). For now, I just pined the commit version of the xiilib library, but I will update that repository to make it more formal.

Understood. Thank you!

@jdkandersson
Copy link
Contributor

I have seen this PR, just holding off review as we are still discussing a spec for how we approach this and what the contents of charm.py are

jdkandersson
jdkandersson reviewed Nov 27, 2023
View reviewed changes
src/charm.py Outdated Show resolved Hide resolved
src/charm.py
@@ -33,110 +23,6 @@ def __init__(self, *args: typing.Any) -> None:
args: passthrough to CharmBase.
"""
super().__init__(*args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like some more changes are needed here

@github-actions GitHub Actions
Copy link
Contributor

Test coverage for b5a1828

Name                                                                    Stmts   Miss Branch BrPart  Cover   Missing
-------------------------------------------------------------------------------------------------------------------
.tox/unit/lib/python3.10/site-packages/xiilib/database_migration.py        50      0      6      0   100%
.tox/unit/lib/python3.10/site-packages/xiilib/databases.py                 37      2     11      1    94%   84-85
.tox/unit/lib/python3.10/site-packages/xiilib/exceptions.py                 5      0      0      0   100%
.tox/unit/lib/python3.10/site-packages/xiilib/flask/__init__.py             1      0      0      0   100%
.tox/unit/lib/python3.10/site-packages/xiilib/flask/charm.py               83     16     10      3    77%   54-57, 123-124, 126-127, 147->exit, 155-156, 160-161, 212, 215, 218, 221
.tox/unit/lib/python3.10/site-packages/xiilib/flask/charm_state.py         97      5     16      3    91%   29, 216-218, 321
.tox/unit/lib/python3.10/site-packages/xiilib/flask/constants.py            8      0      0      0   100%
.tox/unit/lib/python3.10/site-packages/xiilib/flask/flask_app.py           50      2     18      2    94%   69->71, 106-107
.tox/unit/lib/python3.10/site-packages/xiilib/flask/secret_storage.py      14      1      0      0    93%   38
.tox/unit/lib/python3.10/site-packages/xiilib/observability.py             20      0      0      0   100%
.tox/unit/lib/python3.10/site-packages/xiilib/secret_storage.py            41      6     16      4    79%   55, 59->58, 60->62, 90, 108-113
.tox/unit/lib/python3.10/site-packages/xiilib/webserver.py                 84      5     12      1    94%   70, 217, 223-229
-------------------------------------------------------------------------------------------------------------------
TOTAL                                                                     490     37     89     14    90%

Static code analysis report

Run started:2023-11-30 02:13:44.512785

Test results:
  No issues identified.

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

@canonical canonical deleted a comment from jdkandersson Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@arturo-seijas arturo-seijas arturo-seijas left review comments

@jdkandersson jdkandersson jdkandersson left review comments

@yanksyoon yanksyoon yanksyoon left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Libraries: Out of sync trivial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@weiiwang01 @arturo-seijas @jdkandersson @yanksyoon