-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: flask and django, update to paas-app-charmer 1.0.3 #1691
feat: flask and django, update to paas-app-charmer 1.0.3 #1691
Conversation
@javierdelapuente please ping me when this is ready to review |
Hi @lengau , can you have a look at this PR? Thanks! |
@@ -134,6 +134,17 @@ def _get_root_snippet(self) -> dict[str, Any]: | |||
"description": f"{self.framework} application image.", | |||
}, | |||
}, | |||
"charm-libs": [ | |||
{"lib": "traefik_k8s.ingress", "version": "2"}, | |||
{"lib": "observability_libs.juju_topology", "version": "0"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any worry with version 0 libs here that they'll make breaking changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for some reason it is common to start the versions on version 0 for charm libraries, I have seen it a lot. There generally shouldn't be breaking changes within any major version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully LIBPATCH will not introduce non-breaking changes. https://juju.is/docs/sdk/document-your-charm-library.
I do not think the LIBAPI 0 has the usual semantics of semver with major number 0 (that any breaking change can happen). In the wild, we haven't had problems with that either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! I have one question but it's for my own understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with the understanding that:
- charms using these extensions with
charm-libs
defined in acharmcraft.yaml
will now get an error - there is not a mechanism to override or modify the charm-libs defined by the extension
- this can be added later as a feature
- these extensions are experimental, so this is an OK change
I will add a new PR with a fix and a test. As you point out, the error is not nice and no merging is possible. Thanks. |
As pointed by @mr-cal in #1691 (review), the Flask extension fails when a `charm-libs` is already defined in a `charmcraft.yaml`, with a not very clear message because of an `AttributeError` exception. This PR allows having a `charm-libs` in the original `charmcraft.yaml`, and will combine both `charm-libs` lists (if there are duplicated elements they will be only downloaded once). Co-authored-by: Alex Lowe <alex.lowe@canonical.com>
Version 1.0.3 of paas-app-charmer has been released, providing the new following functionality:
charmcraft.yaml
file is set to false and the integration is not active, paas-app-charmer will block the charm (and stop all the services).This PR adapts charmcraft to get advantage of that functionality.
Besides those changes,
charm-libs
has been added to the extension (including the new two new libraries for SAML and S3). That way the user can docharmcraft fetch-libs
instead of onefetch-lib
per library.