extensions: new flask extension #317

weiiwang01 · 2023-07-05T04:47:42Z

This pull request introduces an experimental Flask extension for the Juju Platform as a Service ecosystem in rockcraft.
The extension aims to facilitate the deployment and management of Flask-based web applications on Juju PaaS.

Here's an example project that uses the flask-extension https://github.com/weiiwang01/dqlite.io/blob/flask-extension/rockcraft.yaml.

[] Have you signed the CLA?

rockcraft/extensions/paas_flask.py

rockcraft/extensions/__init__.py

rockcraft/extensions/paas_flask.py

sergiusens · 2023-07-10T16:33:53Z

rockcraft/extensions/paas_flask.py

+            "paas-flask/install-app": {
+                "plugin": "dump",
+                "source": ".",
+                "organize": renaming_map,
+                "stage": list(renaming_map.values()),


how is this supposed to work? Were we not supposed to use the npm target for this?

We should add some minimal documentation on how to work with this

This approach aligns with the web team's requirements, as they prefer to construct the static assets themselves before executing the rockcraft pack command. And let rockcraft should incorporate the built artifact into the OCI image without making any modifications. I will add some comments about this.

For flask, developers don't use npm for static assets since flask is a server side rendered framework primarily. Hence we decided not to add support for npm based static assets in the flask extension. For others it could make sense or those assets should be served, e.g., using a content cache instead or dedicated 12-factor extension

sergiusens · 2023-07-14T12:44:03Z

That'a perfecly reasonable explanation. I would still like to see a spread test for this and perhaps some documentation. Documentation can be a separate PR, but a spread test show casing this seems like a must.

weiiwang01 · 2023-07-17T12:04:05Z

That'a perfecly reasonable explanation. I would still like to see a spread test for this and perhaps some documentation. Documentation can be a separate PR, but a spread test show casing this seems like a must.

Spread test added, thanks!

tests/spread/general/extension-flask/rockcraft.yaml

rockcraft/extensions/flask.py

sergiusens · 2023-08-10T12:40:27Z

docs/how-to/use-flask-extension.rst

@@ -0,0 +1,58 @@
+The Flask Extension


this looks like a good start, it does however mixup reference, explanation, tutorial and how-to

ref: https://diataxis.fr/

Gotcha, I have splited the document into a "how-to" and a "reference".
I also added the services field to the extension as well following last week's discussion.

weiiwang01 · 2023-08-28T17:15:17Z

@tigarmo I have updated the pull request with some unit tests for the flask extension.

tigarmo

Thanks! I haven't finished reviewing yet, will pick this up again in a little bit

docs/how-to/index.rst

docs/how-to/use-flask-extension.rst

tigarmo · 2023-08-30T10:54:59Z

docs/how-to/use-flask-extension.rst

+
+.. code-block:: yaml
+
+    name: example


I think we can improve the fields a bit, even if they are just an example:

name: example-flask summary: A Flask application description: A ROCK packing a Flask application via the flask extension

Yes, indeed, updated, thanks!

tigarmo · 2023-08-30T11:09:41Z

docs/reference/extensions.rst

@@ -0,0 +1,16 @@
+Extensions


👍 ty for starting this page

docs/reference/rockcraft.yaml.rst

tigarmo · 2023-08-30T11:11:10Z

rockcraft/extensions/__init__.py

@@ -26,3 +26,7 @@
    "register",
    "unregister",
 ]
+
+from .flask import Flask as _Flask


why the rename?

I am not exactly sure why, but indeed it's unnecessary, updated, thanks!

tigarmo · 2023-08-30T11:38:15Z

rockcraft/extensions/flask.py

+            snippet["flask/container-processing"] = {
+                "plugin": "nil",
+                "source": ".",
+                "override-prime": "craftctl default\nmkdir -p tmp\nchmod 777 tmp",


a bit surprised that this works, I guess craftctl default is changing the current dir to CRAFT_PRIME?
regardless, I think it's cleaner/more idiomatic to do something like "override-build": "mkdir -m 777 ${CRAFT_PART_INSTALL}/tmp"

That's more cleaner, thanks, updated!

tigarmo · 2023-08-30T11:38:48Z

rockcraft/extensions/flask.py

+
+    @override
+    def get_parts_snippet(self) -> Dict[str, Any]:
+        """Create necessary parts to facilitate the flask application."""


I guess this docstring is redundant/misleading

Yes, I forgot to updated the docstring, updated, thanks!

Co-authored-by: Tiago Nobrega <[email protected]>

tigarmo

Sorry for the delay, now I'm done!

tigarmo · 2023-08-31T11:37:09Z

rockcraft/extensions/flask.py

+                "working-dir": "/srv/flask/app",
+            }
+        }
+        existing_services = self.yaml_data.get("services", {})


I think for consistency with the part handling this should also be a deepcopy of the existing data

Added, perhaps we should define the yaml_data as a property method. This way, we can prevent such issues at the framework level.

@property def yaml_data(self) -> Dict[str, Any]: return self._yaml_data

tests/spread/general/extension-flask/task.yaml

tigarmo · 2023-08-31T11:45:08Z

tests/unit/extensions/test_extensions.py

+        "srv/flask/app/requirements.txt",
+        "srv/flask/app/static",
+    ]
+    del parts["flask/install-app"]["stage"]


an alternative to sorting + deleting it here, which I assume you did because of the unpredictable ordering, is to have the extension always sort the list of stage files. Then it's more deterministic which is a plus

Good idea, updated, thanks!

tigarmo · 2023-08-31T11:48:21Z

tests/unit/extensions/test_extensions.py

+
+    input_yaml["extensions"] = ["flask"]
+    input_yaml["parts"] = {"flask/install-app": {"prime": ["-srv/flask/app/foobar"]}}
+    input_yaml["services"] = {


possibly add another service here, to test that this new service is preserved

Added, thanks!

tigarmo · 2023-08-31T11:49:26Z

rockcraft/extensions/flask.py

+            - flask/install-app: copy the flask project into the OCI image
+        """
+        if not (self.project_root / "requirements.txt").exists():
+            raise ValueError(


I think errors.ExtensionError is more specific here

Indeed, updated, thanks!

tigarmo · 2023-08-31T11:57:00Z

tests/unit/extensions/test_extensions.py

@@ -134,3 +134,102 @@ def test_project_load_extensions(fake_extensions, tmp_path):

    # New part
    assert parts[f"{FullExtension.NAME}/new-part"] == {"plugin": "nil", "source": None}
+
+
+def test_flask_extensions(fake_extensions, tmp_path, input_yaml, monkeypatch):


Thanks for the tests! I think a couple are missing:

A test that checks that apply_extensions() fails if requirements.txt is missing

A test that checks that build_base: ubuntu:22.04 gets added if the key is missing and base is bare

Since the extension has a custom merge for the parts, a test that merges existing keys. For example: a yaml that has:

parts: flask/dependencies: python-requirements: ["requirements-jammy.txt"]

In light of this, I think all of these tests (and these new ones) should be moved to a test_flask module in unit/extensions

Gotcha, I have included more tests and move they to the test_flask.py, thanks!

jdkandersson · 2023-09-01T03:41:07Z

docs/how-to/use-flask-extension.rst

+
+Example:
+
+.. code-block:: yaml


Check that this meets all the requirements, e.g., we have made the prime required now

Documents has been updated, thanks!

jdkandersson · 2023-09-01T03:43:37Z

rockcraft/extensions/flask.py

+import copy
+import os
+import posixpath
+from typing import Any, Dict, Optional, Tuple


Which python version is used on rockcraft? Some of these are deprecated and should be replaced with the built in types dict, tuple etc

rockcraft is targeting python>=3.8

@jdkandersson is right with regards to the current best practices but the rest of rockcraft's codebase is still using the types from typing, so I suggest we keep this for consistency and then in a separate issue we can convert all code

jdkandersson · 2023-09-01T04:52:07Z

rockcraft/extensions/flask.py

+        ignores = [".git", "node_modules", ".yarn"]
+        source_files = [
+            f
+            for f in os.listdir(self.project_root)


what about using pathlib?

Updated, thanks!

jdkandersson · 2023-09-01T04:54:21Z

rockcraft/extensions/flask.py

+            for f in os.listdir(self.project_root)
+            if f not in ignores and not f.endswith(".rock")
+        ]
+        renaming_map = {f: posixpath.join("srv/flask/app", f) for f in source_files}


I was trying to find the handling of requiring prime and checking that everything in there has the srv/flask/app prefix?

That has been added in the latest commit, thanks!

tigarmo

thanks for all this work @weiiwang01

tigarmo · 2023-09-14T13:41:39Z

rockcraft/extensions/flask.py

+import copy
+import os
+import posixpath
+from typing import Any, Dict, Optional, Tuple


@jdkandersson is right with regards to the current best practices but the rest of rockcraft's codebase is still using the types from typing, so I suggest we keep this for consistency and then in a separate issue we can convert all code

tigarmo · 2023-09-14T13:44:12Z

tests/spread/general/extension-flask/task.yaml

@@ -25,6 +25,11 @@ execute: |

  # test the part merging
  docker run --rm --entrypoint /bin/python3 flask-extension -c "import pathlib;assert not pathlib.Path('/srv/flask/app/README').exists()"
+
+  # test the default flask service
+  docker run --rm --name flask-extension-container -d -p 8137:8000 flask-extension


we should remove this container in the restore step
docker rm -f flask-extension-container

Added, thanks!

tigarmo · 2023-09-14T13:46:38Z

tests/unit/extensions/test_flask.py

+
+
+@pytest.mark.usefixtures("flask_extension")
+def test_flask_extension_error(tmp_path):


thanks for testing the error cases! I think we should split this into separate tests. It's a bit of work but it makes each individual test easier to reason about, and lowers the chance that they might interfere with each other in unexpected ways

Gotcha, separate them into different test cases with appropriate names, thanks!

docs/how-to/use-flask-extension.rst

sergiusens · 2023-09-20T18:17:32Z

when this is merged, a snap of rockcraft with this implementation will show up as edge/12f (the feature branch leaf name)

docs/how-to/use-flask-extension.rst

sergiusens · 2023-09-21T12:02:20Z

rockcraft/extensions/flask.py

+                "missing requirements.txt file, "
+                "flask extension requires this file with flask specified as a dependency"
+            )
+        source_files = [f.name for f in sorted(self.project_root.iterdir())]


we can go with this for now, but this extension as is, only serves 12f, as one cannot have https://github.com/flaskfactory/my-flask.git as source entry for building your application

tigarmo · 2023-09-28T11:56:38Z

well done @weiiwang01 !

This commit is the squashed version of the long-lived `feature/12f` branch containing the most recent version of the Flask extension in the context of the 12-factor App methodology. The original commit log is kept here for posterity, and follows: ------ * feat: new flask extension (#317) Co-authored-by: Sergio Schvezov <[email protected]> Co-authored-by: Tiago Nobrega <[email protected]> * feat: provide a default prime in the flask extension (#374) * feat(extensions): add a profile option to the rockcraft init command (#379) * chore(deps): update craft-providers to 1.19.2 The current version has two bugs (the "looping timer" one and the "not collecting error logs" one) that are blocking for many users. Ideally we should merge main into this branch, this is a stopgap before we can allocate time for it. * chore: manual updates to align with origin/main These are related to the change in base notation (ubuntu@ instead of ubuntu:) and the changes due to general craft-application-flow. * fix(docs): remove duplicate how-to entries * Fix schema validation * Fix typo * Template requires input variables * Fix lint * Fix lint * Update flask-framework extension * Remove support for ubuntu:20.04 * ROCK to rock and update copyright info * Update flask doc with real examples * Update use-flask-extension.rst * Update pyright exclude * Fix some linting issues * enable extension in spread tests * Fix use-flask-extension spread test * Rename ROCK to rock * Apply suggestions from code review Co-authored-by: Alex Lowe <[email protected]> --------- Co-authored-by: Sergio Schvezov <[email protected]> Co-authored-by: Tiago Nobrega <[email protected]> Co-authored-by: Gregory Schiano <[email protected]> Co-authored-by: Alex Lowe <[email protected]>

weiiwang01 added 5 commits July 5, 2023 00:09

Add flask framework extension

3f3a222

Update documents in the flask extension

caa7dbf

Fix some linting issues

128c58d

Fix some linting issues

016b2b1

Update the name of the extension

10ec7d4

sergiusens reviewed Jul 10, 2023

View reviewed changes

rockcraft/extensions/paas_flask.py Outdated Show resolved Hide resolved

sergiusens reviewed Jul 10, 2023

View reviewed changes

rockcraft/extensions/paas_flask.py Outdated Show resolved Hide resolved

sergiusens reviewed Jul 10, 2023

View reviewed changes

rockcraft/extensions/__init__.py Outdated Show resolved Hide resolved

sergiusens reviewed Jul 10, 2023

View reviewed changes

rockcraft/extensions/paas_flask.py Outdated Show resolved Hide resolved

sergiusens reviewed Jul 10, 2023

View reviewed changes

Some document and naming changes

8f622fa

weiiwang01 added 4 commits July 17, 2023 13:42

Add a spread test for the flask extension

d5a048d

raise an error when requirements.txt doesn't exist

dde4b33

Fix the linting issue

f40b7dd

Merge branch 'main' into main

2364f45

weiiwang01 added 2 commits July 21, 2023 11:55

Merge branch 'main' into main

ce8c2b5

Add more items to the flask extension spread test

2f4e560

sergiusens reviewed Jul 26, 2023

View reviewed changes

tests/spread/general/extension-flask/rockcraft.yaml Show resolved Hide resolved

sergiusens reviewed Jul 26, 2023

View reviewed changes

rockcraft/extensions/flask.py Outdated Show resolved Hide resolved

sergiusens and others added 6 commits July 26, 2023 00:07

Merge branch 'main' into main

d6926e3

Add support for 20.04 in flask extension

21d7dfb

Merge branch 'main' into main

2bc660d

Add extensions section in the rockcraft.yaml doc

310408f

Update document and part merging

6f1a627

Only allow to overwrite prime in flask/install-app

8c25807

sergiusens changed the title ~~Add experimental flask extension support for juju paas~~ extensions: new flask extension Aug 10, 2023

sergiusens reviewed Aug 10, 2023

View reviewed changes

Update part merging and documents

1e8f860

weiiwang01 added 4 commits August 23, 2023 01:12

Change to British English and update wordlist

75049de

Add some unit tests for flask extension

10ec23d

Fix the flask extension on windows

3212eae

Merge branch 'main' into main

0fe5723

tigarmo requested changes Aug 30, 2023

View reviewed changes

weiiwang01 and others added 3 commits August 31, 2023 14:08

Apply suggestions from code review

ce2ce3e

Co-authored-by: Tiago Nobrega <[email protected]>

Apply suggestions from code review

6e3f682

Update documents

463e298

tigarmo requested changes Aug 31, 2023

View reviewed changes

jdkandersson reviewed Sep 1, 2023

View reviewed changes

weiiwang01 added 4 commits September 1, 2023 14:46

Mandate the prime in flask/install-app

cab47e8

Add flask extension service overwrite test

b89ceb7

Fix linting and spread

bf684a3

Fix the shellcheck linting problem

ce9e067

tigarmo requested changes Sep 14, 2023

View reviewed changes

sergiusens changed the base branch from main to feature/12f September 20, 2023 11:04

Apply suggestions from reviews

3841e78

tigarmo reviewed Sep 21, 2023

View reviewed changes

docs/how-to/use-flask-extension.rst Outdated Show resolved Hide resolved

sergiusens reviewed Sep 21, 2023

View reviewed changes

weiiwang01 added 5 commits September 25, 2023 16:10

Update the flask extension design

2340cb6

Fix some linting issues

5487cb5

Update test_flask_framework.py

1df6a35

Update flask_framework.py

5d3a099

Remove working-dir from flask service

c51286e

tigarmo approved these changes Sep 28, 2023

View reviewed changes

tigarmo merged commit e2d7756 into canonical:feature/12f Sep 28, 2023



		@pytest.mark.usefixtures("flask_extension")
		def test_flask_extension_error(tmp_path):

extensions: new flask extension #317

extensions: new flask extension #317

Conversation

weiiwang01 commented Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiusens commented Jul 14, 2023 via email

weiiwang01 commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiiwang01 commented Aug 28, 2023

tigarmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigarmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigarmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiusens commented Sep 20, 2023

Choose a reason for hiding this comment

tigarmo commented Sep 28, 2023

weiiwang01 commented Jul 5, 2023 •

edited

Loading