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.

Sign up for GitHub

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

Jump to bottom

feat(application): merge commands within groups #545

Merged
merged 5 commits into from
Oct 24, 2024
Merged

feat(application): merge commands within groups #545

merged 5 commits into from
Oct 24, 2024

Conversation

mr-cal
Copy link
Contributor

@mr-cal mr-cal commented Oct 23, 2024 •
edited
Loading

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

Allows Snapcraft to override default commands like the InitCommand and PackCommand without a hacky override (source).

Features

  • Applications can add a command to a CommandGroup where craft-application already provides a default command with the same name. The application's command will replace the default command.

Changes

  • CommandGroup.ordered is no longer discarded.
  • The Lifecycle CommandGroup is ordered.
  • If an application registers multiple CommandGroups with the same group name, they will not be merged.
    • I'm OK with this change because it is an odd thing to do, no applications do it, and I would consider it an incorrect way to use craft application.

(CRAFT-3543)

@mr-cal mr-cal mentioned this pull request Oct 23, 2024
Merged
4 tasks
@mr-cal
feat(application): merge commands within groups
bf920bb 
Craft Application will only add default commands in a group if the application
doesn't define the command. The order of command groups is now preserved.

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
@mr-cal mr-cal force-pushed the work/CRAFT-3543-use-app-commands branch from 23bb869 to bf920bb Compare October 23, 2024 17:16
dariuszd21
dariuszd21 reviewed Oct 23, 2024
View reviewed changes
craft_application/application.py Outdated Show resolved Hide resolved
craft_application/application.py Show resolved Hide resolved
mr-cal added 2 commits October 23, 2024 13:18
@mr-cal
chore: feedback from PR
bc0387b 
Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
@mr-cal
style: use debug messages, log less stuff
2961ba0 
Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
dariuszd21
dariuszd21 approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@dariuszd21 dariuszd21 left a comment

Choose a reason for hiding this comment

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

Great!

tigarmo
tigarmo approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@tigarmo tigarmo left a comment

Choose a reason for hiding this comment

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

nice! thanks

craft_application/application.py Outdated Show resolved Hide resolved
@mr-cal mr-cal added the squash label Oct 23, 2024
mr-cal and others added 2 commits October 23, 2024 14:36
@mr-cal
style: drop extra import
26825c9 
Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
@mr-cal
Merge branch 'main' into work/CRAFT-3543-use-app-commands
5957ed1
@mr-cal mr-cal merged commit f618700 into main Oct 24, 2024
9 of 10 checks passed
@mr-cal mr-cal deleted the work/CRAFT-3543-use-app-commands branch October 24, 2024 22:30
linostar pushed a commit to linostar/craft-application that referenced this pull request Dec 4, 2024
@mr-cal @linostar
feat(application): merge commands within groups (canonical#545)
854813c 
Craft Application will only add default commands in a group if the application
doesn't define the command. The order of command groups is now preserved.

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariuszd21 dariuszd21 dariuszd21 approved these changes

@tigarmo tigarmo tigarmo approved these changes

@lengau lengau Awaiting requested review from lengau

Assignees
No one assigned
Labels
squash
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mr-cal @tigarmo @dariuszd21