refactor: use init command from craft-application #5129

mr-cal · 2024-10-23T17:04:50Z

Have you followed the guidelines for contributing?
Have you signed the CLA?
Have you successfully run tox run -m lint?
Have you successfully run tox run -e test-py310? (supported versions: py39, py310, py311, py312)

Replace Snapcraft's init implementation with craft-application's implementation. Somehow I added more lines of code 🙃

There are no behavioral changes with just snapcraft init. However, the command now accepts additional parameters added in craft-application: name, profile, and project-dir.

Fixes #5098
(CRAFT-3543)

Signed-off-by: Callahan Kovacs <[email protected]>

mr-cal · 2024-10-23T17:05:37Z

snapcraft/application.py

-    @property
-    def command_groups(self):
-        """Replace craft-application's LifecycleCommand group."""
-        _command_groups = super().command_groups
-        for index, command_group in enumerate(_command_groups):
-            if command_group.name == "Lifecycle":
-                _command_groups[index] = cli.CORE24_LIFECYCLE_COMMAND_GROUP
-
-        return _command_groups


I would have had to do more hacks for InitCommand, hence canonical/craft-application#545.

Signed-off-by: Callahan Kovacs <[email protected]>

mr-cal · 2024-10-23T21:56:34Z

snapcraft/commands/init.py


+        if parsed_args.name:
+            craft_cli.emit.progress(
+                "Ignoring '--name' parameter because it is not supported yet.",


I'm not sure if this should be an error, a warning, or if I should change the default template to use this as the snap name.

if you changed the default template to support the name, would you be able to drop this class wholesale?

No. The other extra things that snapcraft adds are:

checking for a project file under snapcraft.yaml, snap/snapcraft.yaml, build-aux/snap/snapcraft.yaml or .snapcraft.yaml

An emit message pointing to documentation.

I think the first is worth keeping. The second could be added as a comment to the snapcraft.yaml file.

Alright. Then I think it should be an error and we should plan to address it soon

I'd vote for starting to support the name from the start, and address the rest in the separate item.

Out of curiosity, shouldn't you override the InitService and use the command from upstream (if you ignore the documentation message)?

tigarmo · 2024-10-24T12:38:28Z

requirements-devel.txt

@@ -24,9 +24,9 @@ click==8.1.7
 codespell==2.3.0
 colorama==0.4.6
 coverage==7.6.1
-craft-application==4.2.6
+git+https://github.com/canonical/craft-application@work/CRAFT-3543-use-app-commands#egg=craft-application


reminder to update this

tigarmo · 2024-10-24T12:40:10Z

snapcraft/commands/init.py


+        if parsed_args.name:
+            craft_cli.emit.progress(
+                "Ignoring '--name' parameter because it is not supported yet.",


if you changed the default template to support the name, would you be able to drop this class wholesale?

tigarmo · 2024-10-24T13:41:00Z

snapcraft/commands/init.py

        try:
-            project = get_snap_project()
+            craft_cli.emit.progress("Checking for an existing 'snapcraft.yaml'.")
+            project = get_snap_project(project_dir)


this one sounds like maybe it should go on Snapcraft's (new) InitService
(we can also do this later)

tigarmo · 2024-10-24T13:45:49Z

snapcraft/commands/init.py


-        emit.message(f"Created {str(snapcraft_yaml_path)!r}.")
-        emit.message(
+        craft_cli.emit.message(


it also feels like maybe this message should be on the initservice; or maybe a way for the service to give a template-based string to be presented to the user after init is done (this would be useful in rockcraft, which has per-profile docs)
(we can also do this later)

dariuszd21

Looks good!

dariuszd21 · 2024-10-24T13:44:24Z

snapcraft/commands/init.py


+        if parsed_args.name:
+            craft_cli.emit.progress(
+                "Ignoring '--name' parameter because it is not supported yet.",


I'd vote for starting to support the name from the start, and address the rest in the separate item.

Out of curiosity, shouldn't you override the InitService and use the command from upstream (if you ignore the documentation message)?

dariuszd21 · 2024-10-24T13:48:58Z

tests/spread/general/init/task.yaml

+  fi
+


Suggested change

fi

fi

snapcraft clean

dariuszd21 · 2024-10-24T13:53:52Z

tests/spread/general/init/task.yaml

+  PROFILE/default_profile: null
+  PROFILE/simple_profile: simple
+  PROFILE: null
+  PROJECT_DIR/default_dir: null
+  PROJECT_DIR/project_dir: test-project-dir
+  PROJECT_DIR: null


I recently realized that in that case it will be one test case per variant, so it won't create a combination of simple_profile with project_dir set. Is that ok ?

It will create 4 variant's: default_profile, simple_profile, project_dir, default_dir.
In that case if I can see correctly default_profile and default_dir will be the same test with different name

spread variants are so annoying. I also only semi-recently internalized that this:

environment: VAR1/bla: var1-bla VAR1/ble: var1-ble VAR2/bla: var2-bla VAR2/ble: var2-ble

can be thought of as:

environment: bla: VAR1: var1-bla VAR2: var2-bla ble: VAR1: var1-be VAR2: var2-ble

build(deps): bump craft-application, craft-cli

c52d04e

Signed-off-by: Callahan Kovacs <[email protected]>

mr-cal commented Oct 23, 2024

View reviewed changes

mr-cal force-pushed the work/CRAFT-3543/init-command branch 4 times, most recently from c11a314 to d235c56 Compare October 23, 2024 19:26

refactor: use init command from craft-application

57d4325

Signed-off-by: Callahan Kovacs <[email protected]>

mr-cal force-pushed the work/CRAFT-3543/init-command branch from d235c56 to 57d4325 Compare October 23, 2024 20:38

mr-cal requested review from tigarmo and dariuszd21 October 23, 2024 21:53

mr-cal commented Oct 23, 2024

View reviewed changes

tigarmo approved these changes Oct 24, 2024

View reviewed changes

tigarmo reviewed Oct 24, 2024

View reviewed changes

dariuszd21 approved these changes Oct 24, 2024

View reviewed changes

mr-cal marked this pull request as draft October 28, 2024 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use init command from craft-application #5129

refactor: use init command from craft-application #5129

mr-cal commented Oct 23, 2024

mr-cal Oct 23, 2024

mr-cal Oct 23, 2024

tigarmo Oct 24, 2024

mr-cal Oct 24, 2024

tigarmo Oct 24, 2024

dariuszd21 Oct 24, 2024

tigarmo Oct 24, 2024

tigarmo Oct 24, 2024

tigarmo Oct 24, 2024

tigarmo Oct 24, 2024

dariuszd21 left a comment

dariuszd21 Oct 24, 2024

dariuszd21 Oct 24, 2024

dariuszd21 Oct 24, 2024

tigarmo Oct 24, 2024

refactor: use init command from craft-application #5129

Are you sure you want to change the base?

refactor: use init command from craft-application #5129

Conversation

mr-cal commented Oct 23, 2024

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

dariuszd21 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