Skip to content
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

Add init template for flask-framework #1428

Merged
merged 7 commits into from
Dec 14, 2023
Merged

Add init template for flask-framework #1428

merged 7 commits into from
Dec 14, 2023

Conversation

weiiwang01
Copy link
Contributor

Introduce a new profile called flask-framework to the charmcraft init command to create basic projects for 12-factor Flask charms

@weiiwang01
Copy link
Contributor Author

I have chosen to omit certain files, such as tests and tox.ini, from the template. This decision is based on the typical use cases where the 12-factor charm is located within a charm/ directory in the user's project file. In most instances, the test and tox files are placed at the root of the project.

@@ -0,0 +1 @@
https://github.com/canonical/xiilib/archive/v0.1.0.tar.gz
Copy link
Member

Choose a reason for hiding this comment

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

Out of interest, what's the name? xiilib? Feels a bit cryptic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was originally going to be 12f, but names starting with a number caused some problems, so I substituted it with number XII.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any great ideas for better names are more than welcome 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

how about twelve-f? Looks awkward at first glance but reads the same as 12f

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll throw my hat in the ring with zwolf. Advantages:

  • Is the German word for twelve
  • Isn't yet registered on pypi
  • Includes the substring "wolf" 🐺

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about ffffffffffff, and it's pronounced as /ɛfɛfɛfɛfɛfɛfɛfɛfɛfɛfɛfɛf/ 🤣

@@ -0,0 +1,30 @@
#!/usr/bin/env python3
# Copyright 2023 Canonical Ltd.
Copy link
Member

Choose a reason for hiding this comment

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

The author/year needs to be templated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

Comment on lines +24 to +31
# Uncomment the integrations used by your application
# requires:
# mysql:
# interface: mysql_client
# limit: 1
# postgresql:
# interface: postgresql_client
# limit: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this include all of the integrations we support? E.g., ingress seems to be missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ingress and other relations are default and static for now, so user can't change them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see

super().__init__(*args)


if __name__ == "__main__": # pragma: nocover
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the pragma: nocover? It might not be relevant for all users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

@@ -0,0 +1 @@
https://github.com/canonical/xiilib/archive/v0.1.0.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Any great ideas for better names are more than welcome 🙂

Copy link
Collaborator

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Looks good. I shared some non-blocker opinions too

@@ -35,6 +35,7 @@
"simple": "init-simple",
"kubernetes": "init-kubernetes",
"machine": "init-machine",
"flask-framework": "init-flask-framework",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update _overview below to include a description of this template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added an one sentence summary for now, but I will add a link to the document once we have published the document for the 12-factor project.

mkdir -p test-init
cd test-init
charmcraft init --profile flask-framework
export CHARMCRAFT_ENABLE_EXPERIMENTAL_EXTENSIONS=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be set in an environment keyword for the task. Here's an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

@@ -0,0 +1,31 @@
# This file configures Charmcraft.
# See https://juju.is/docs/sdk/charmcraft-config for guidance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While the extension does fill in the type field, I'd prefer if we keep it in the template too, since a user might see from the documentation that it's required and be confused by it. This confusion will become more obvious when we make a jsonschema for charmcraft.yaml, since that will mark type as required (and thus IDEs will start complaining about its nonexistence).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, added, thanks!

@@ -0,0 +1 @@
https://github.com/canonical/xiilib/archive/v0.1.0.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

how about twelve-f? Looks awkward at first glance but reads the same as 12f

@tigarmo tigarmo merged commit cc670c9 into canonical:feature/12f Dec 14, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants