feat(profiles): bound dependencies, remove uv.lock files, improve init output#2754
feat(profiles): bound dependencies, remove uv.lock files, improve init output#2754dwilding wants to merge 13 commits into
Conversation
|
|
||
| # The charm itself doesn't matter, so replace the dependencies with something | ||
| # simple and small so we don't have to build pyyaml. | ||
| # NOTE: We still do need at least one dependency here so we can trigger the cache. | ||
| echo "distro" > requirements.txt |
There was a problem hiding this comment.
This part of the test has been unused since we migrated the kubernetes profile to use pyproject.toml for dependencies. (Likely I missed updating the test at the time.) Unless anyone objects, I think we can drop it.
6a7dba9 to
4977ca8
Compare
4977ca8 to
e10c029
Compare
| @@ -31,71 +31,67 @@ extension. | |||
|
|
|||
| .. dropdown:: Example | |||
There was a problem hiding this comment.
The example seems out of place. It's in a section called "View details about the extension in use", and I think the example is there to illustrate the "Suppose you've initialised [...]" part. But this seems distracting. We already have exactly the same example on the "Manage charms" page, so maybe the best thing would be to remove the example from this page?
Maybe we only keep the part that shows charmcraft.yaml, since that is the main point of the text:
[...] your rockcraft.yaml / charmcraft.yaml > extensions lists this extension.
There was a problem hiding this comment.
That seems reasonable to me. Since the section is about extensions that are already in use, the example output you point out should've come and gone well before the user gets here.
This page needs more attention, but I think cutting this is a fine way to start :)
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
Thanks! I only tried a couple of profiles myself, but they worked well, and the rest make sense to me reading through it. A few small questions/suggestions.
| ├── pyproject.toml - Configuration for testing, formatting and | ||
| │ linting tools. Specifies Python dependencies for | ||
| │ all profiles except 12-factor app profiles | ||
| │ targeting Ubuntu 24.04 or lower |
There was a problem hiding this comment.
lower->earlier? Same below.
There was a problem hiding this comment.
'higher' and 'lower' are preferred in the craft apps https://documentation.ubuntu.com/starflow/latest/how-to/starcraft-style-guide/#version-names
However, we should change 'Ubuntu 24.04' to 'Ubuntu 24.04 LTS' for branding and consistency with the subsequent change to this file.
| │ linting tools. Specifies Python dependencies for | ||
| │ all profiles except 12-factor app profiles | ||
| │ targeting Ubuntu 24.04 or lower | ||
| │ your charm if profile is 'kubernetes' or 'machine' |
There was a problem hiding this comment.
Is this line meant to be removed?
| else: | ||
| return default_message |
There was a problem hiding this comment.
Very much just style, so feel free to ignore.
| else: | |
| return default_message | |
| return default_message |
| # then inspect the libraries for dependencies specified in PYDEPS. List those dependencies here. | ||
| dependencies = [ | ||
| "ops~=3.7", | ||
| "ops>=3.8,<4", |
There was a problem hiding this comment.
I know not everyone agrees, and it's annoying that there isn't "one obvious way to do it", but I think ~ is a nicer way to express this.
| "ruff", | ||
| "codespell", | ||
| "pyright", | ||
| "ruff>=0.15,<1", |
There was a problem hiding this comment.
I'm not sure <1 makes sense. Generally, a 0.x release has no API guarantee at all, so any version bump contains the same risk. Going from 0.x to 1.0 actually seems likely to increase stability in terms of the API, because it should be saying "ok, we're committed to this API now, and will maintain it until there's a breaking 2.x release".
No upper bound is probably not ideal either. ~0.15 would give the intent better, but assuming we're not using ~, what about making it >=0.15<2?
| "ops[testing]", | ||
| "pytest", | ||
| "coverage[toml]>=7.15,<8", | ||
| "ops[testing]>=3.8,<4", |
There was a problem hiding this comment.
We don't really need the versions here, because ops will force them. I'm not sure what uv add and the like will do here, but I think generally it would be nicer to have no constraint here, and perhaps a comment explaining why. That way this doesn't need to be kept in lockstep to avoid impossible resolution.
| if uses_uv: | ||
| assert "\nuv.lock\n" not in output # uv.lock isn't listed as a created file. | ||
| assert "Run 'uv lock'" in output | ||
| else: | ||
| assert "uv.lock" not in output |
There was a problem hiding this comment.
I get that these are doing different things - not being a listed file and not being present at all, but it seems like a very subtle difference for the test, and maybe a bit fragile given that they differ only by whitespace?
I guess this line can't do the work for both because there's an instruction to commit uv.lock that would be hit in that case. What about having the first one be a regex that matches being in the list of files?
| snap install --classic --no-wait astral-uv | ||
| snap watch --last=install | ||
| charmcraft init --project-dir=charm | ||
| cd charm |
There was a problem hiding this comment.
Is it safe here to change into the charm directory and not change out? I'm not sure exactly what prepare does at the end, if it restores you to where you were for example. It seems like the pushd/popd style used elsewhere might be cleaner.
|
|
||
| When a charm is initialized with the Kubernetes or machine profile, Charmcraft creates | ||
| this file. | ||
| Charmcraft doesn't create this file. |
There was a problem hiding this comment.
Does this need to be a paragraph of its own? It seems like it could be the intro for the next one.
jahn-junior
left a comment
There was a problem hiding this comment.
Thanks, @dwilding! I left a bunch of minor wording tweaks and one minor error correction, but none of them are blocking.
Feel free to accept, ignore, or tweak these suggestions as you see fit.
| ├── pyproject.toml - Configuration for testing, formatting and | ||
| │ linting tools. Specifies Python dependencies for | ||
| │ all profiles except 12-factor app profiles | ||
| │ targeting Ubuntu 24.04 or lower |
There was a problem hiding this comment.
'higher' and 'lower' are preferred in the craft apps https://documentation.ubuntu.com/starflow/latest/how-to/starcraft-style-guide/#version-names
However, we should change 'Ubuntu 24.04' to 'Ubuntu 24.04 LTS' for branding and consistency with the subsequent change to this file.
| Next steps: | ||
|
|
||
| 1. Run 'uv lock' | ||
| 2. Edit charmcraft.yaml and pyproject.toml to provide metadata, then commit (including uv.lock) |
There was a problem hiding this comment.
Optional:
| 2. Edit charmcraft.yaml and pyproject.toml to provide metadata, then commit (including uv.lock) | |
| 2. Edit the charmcraft.yaml and pyproject.toml files to provide metadata, then commit (including uv.lock) |
Since this is in CLI copy, I could go either way it adds characters. Up to you!
| @@ -31,71 +31,67 @@ extension. | |||
|
|
|||
| .. dropdown:: Example | |||
There was a problem hiding this comment.
That seems reasonable to me. Since the section is about extensions that are already in use, the example output you point out should've come and gone well before the user gets here.
This page needs more attention, but I think cutting this is a fine way to start :)
| ``tox.ini`` requires the `tox-uv <https://github.com/tox-dev/tox-uv>`_ plugin. First | ||
| make sure that `uv <https://docs.astral.sh/uv/>`_ is installed on the current host. Then | ||
| use uv to install tox and tox-uv: |
There was a problem hiding this comment.
Various optional nits:
| ``tox.ini`` requires the `tox-uv <https://github.com/tox-dev/tox-uv>`_ plugin. First | |
| make sure that `uv <https://docs.astral.sh/uv/>`_ is installed on the current host. Then | |
| use uv to install tox and tox-uv: | |
| The ``tox.ini`` file requires the `tox-uv <https://github.com/tox-dev/tox-uv>`__ plugin. First, | |
| make sure that `uv <https://docs.astral.sh/uv/>`__ is installed on the current host. Then, | |
| install tox and tox-uv with uv: |
|
|
||
| For 12-factor app profiles, ``tox.ini`` doesn't require any plugins. Dependencies are | ||
| specified in the :ref:`requirements-txt-file` instead of ``pyproject.toml``. | ||
| For 12-factor app profiles targeting Ubuntu 24.04 LTS or lower: ``tox.ini`` doesn't |
There was a problem hiding this comment.
Optional:
| For 12-factor app profiles targeting Ubuntu 24.04 LTS or lower: ``tox.ini`` doesn't | |
| For 12-factor app charms targeting Ubuntu 24.04 LTS or lower, ``tox.ini`` doesn't |
| Charmcraft doesn't create this file. This file is required if your charm uses the | ||
| :ref:`craft_parts_uv_plugin`. It's also required by the tox-uv plugin. For more |
There was a problem hiding this comment.
Optional:
| Charmcraft doesn't create this file. This file is required if your charm uses the | |
| :ref:`craft_parts_uv_plugin`. It's also required by the tox-uv plugin. For more | |
| Charmcraft doesn't create this file. This file is required by the tox-uv plugin and | |
| charms with environments managed by the :ref:`craft_parts_uv_plugin`. For more |
| Use ``uv lock`` to create this file and update it if you change your charm's | ||
| dependencies. Alternatively, you can update this file by using ``uv add`` and | ||
| ``uv remove`` to manage dependencies in the :ref:`pyproject-toml-file`. |
There was a problem hiding this comment.
Optional:
| Use ``uv lock`` to create this file and update it if you change your charm's | |
| dependencies. Alternatively, you can update this file by using ``uv add`` and | |
| ``uv remove`` to manage dependencies in the :ref:`pyproject-toml-file`. | |
| To create or update this file, run the ``uv lock`` command. Alternatively, | |
| you can update this file by running ``uv add`` or ``uv remove`` to | |
| manage dependencies in the :ref:`pyproject-toml-file`. |
|
|
||
| For the 12-factor app profiles, this file isn't necessary. Dependencies are specified | ||
| in the :ref:`requirements-txt-file` instead of ``pyproject.toml``. | ||
| For 12-factor app profiles targeting Ubuntu 24.04 LTS or lower: This file isn't |
There was a problem hiding this comment.
Optional:
| For 12-factor app profiles targeting Ubuntu 24.04 LTS or lower: This file isn't | |
| For 12-factor app charms targeting Ubuntu 24.04 LTS or lower: This file isn't |
|
Thanks for the reviews! I didn't have time today to finish working through the feedback. I'll come back to this after my break (from today until Aug 17). |
|
@dwilding just ping me when this is ready to merge :-) |
This PR makes several changes related to profiles. Fixes #2749.
For the
kubernetesandmachineprofiles, bound all dependencies so that (on user update) they stay within the same major version. The minimum version of each dependency is the latest tested major.minor version. See:charmcraft/templates/init-kubernetes/pyproject.toml.j2charmcraft/templates/init-machine/pyproject.toml.j2Remove
uv.lockfrom thekubernetesandmachineprofiles. After internal discussion, we feel it makes more sense for the user to lock and ensure they have up-to-date dependencies, rather than us continually bumping the template lockfiles.After making this change, I updated the Spread tests as thoroughly as I could.
Make the output of
charmcraft initbetter match the requested profile:flask-frameworkthe summary listsrequirements.txt; fortest-kubernetesthe summary only lists Spread-related files.uv lock. This applies tokubernetes,machine, and the upcoming 26.04 12-factor profiles. It doesn't apply to the existing 12-factor profiles.Example for
kubernetesExample for
flask-frameworkUpdated
initexamples in the documentation:Updated file reference docs to present uv-managed charms as the default, which will automatically cover the 26.04 12-factor profiles when they are released. The existing 12-factor profiles are called out as an exception.
pyproject.tomlfilerequirements.txtfiletox.inifileuv.lockfileI also updated the
initcommand help. Seecharmcraft/application/commands/init.py.For info about the upcoming 26.04 12-factor profiles, see #2742.
make lint && make test.