feat: upstream snap hooks by mattculler · Pull Request #710 · canonical/craft-providers

mattculler · 2025-01-07T18:48:55Z

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

Applications using craft-providers may inadvertently leave around lxc instances, either when new base instances supersede old ones, or when the application is uninstalled. Add a module implementing logic that applications can use in configure and remove snap hooks to prune these unneeded lxc instances.

mr-cal

Looking good!

I'm not sure if the criteria for this task is to upstream or upstream and de-duplicate redundant code. My comments are based on the latter.

The de-duplication is only possible now that this is internal to craft-providers and therefore can import other internal modules.

Either way, I would like to have an integration test for the configure and remove hooks.

craft_providers/hookutil.py

tests/unit/test_hookutil.py

cmatsuoka

The way the module data is organized is making the code hard to read, I think we can refactor this to make it easier for our future selves to rework this if needed. Also please add a description of the problem being solved to the PR so we can check if the implementation is doing everything that it's supposed to do.

craft_providers/hookutil.py

mattculler · 2025-01-10T19:47:03Z

Also please add a description of the problem being solved to the PR so we can check if the implementation is doing everything that it's supposed to do.

@cmatsuoka I reworked the description to add more detail, how does that look now?

mattculler · 2025-01-10T21:12:57Z

Looking good!

I'm not sure if the criteria for this task is to upstream or upstream and de-duplicate redundant code. My comments are based on the latter.

The de-duplication is only possible now that this is internal to craft-providers and therefore can import other internal modules.

Either way, I would like to have an integration test for the configure and remove hooks.

Awesome - based on the task pointage I was just going to upstream, not deduplicate. I also didn't realize there would be so much deduplication possible - thanks for pointing all that out! I'll open another ticket to capture your knowledge drop.

Re: integration tests, the private repo that this originated from has spread tests that do this, I'll see if I can rework those into integration tests here.

Lightly modified to remove proprietary identifiers, not functional in this state.

- This way is also simpler in the code, with a slight performance hit for the regex. - There's no way to get the full compat tag without having a fully-instantiated application. - Also fixed some debug output.

mr-cal

LGTM

cmatsuoka

Thanks! Remaining issues can be addressed in #715.

* feat: mknod intercepts now configurable from LXDProvider * fix: unit test * fix: lint * docs: update changelog (also includes item from #710)

…ks in Rockcraft. (#721) * ci: install lxd on ubuntu 24.04 Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> * ci: install lxd from the candidate channel Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> * docs: remove starter pack submodule (#713) Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> * fix: use Multipass-compatible instance names (#712) Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> * feat: upstream snap hooks (#710) * feat: add hookutil from downstream repo Lightly modified to remove proprietary identifiers, not functional in this state. * feat: make hookutil work generically * chore: disable lint check - may change approach later * feat: add unit tests from downstream * chore: linter issues * fix: craft-providers CI doesn't have lxd, mock it out * chore(style): rename per code review * chore(style): rename per code review * chore(style): rename per code review * chore: update changed names in test * refactor: remove globals, let LXDInstances keep project_name * chore: autoformat * fix: compat tag structure had been dependent on craft-application - This way is also simpler in the code, with a slight performance hit for the regex. - There's no way to get the full compat tag without having a fully-instantiated application. - Also fixed some debug output. * feat(tests): add configure hook integration test * feat: added remove hook test and beefed up configure hook test * chore: autoformat * fix: unit tests for changed interface * chore: autoformat, this time with the other tool! * chore: autoformat the third * fix: use the superior is-installed check from c-prov proper * fix: lint * feat: mknod intercepts now configurable from LXDProvider (#717) * feat: mknod intercepts now configurable from LXDProvider * fix: unit test * fix: lint * docs: update changelog (also includes item from #710) * feat: add modify_file method to Executor class * feat: add properties for getting setting pro services applied to environment * feat: make pull optional in Executor.modify_file * fix(LXDInstance): missing pro_services file returns none * trigger ci * refactor: adjustments to satisfy linters --------- Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> Co-authored-by: Callahan Kovacs <callahan.kovacs@canonical.com> Co-authored-by: Matt Culler <matt.culler@canonical.com>

…ks in Rockcraft. (canonical#721) * ci: install lxd on ubuntu 24.04 Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> * ci: install lxd from the candidate channel Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> * docs: remove starter pack submodule (canonical#713) Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> * fix: use Multipass-compatible instance names (canonical#712) Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> * feat: upstream snap hooks (canonical#710) * feat: add hookutil from downstream repo Lightly modified to remove proprietary identifiers, not functional in this state. * feat: make hookutil work generically * chore: disable lint check - may change approach later * feat: add unit tests from downstream * chore: linter issues * fix: craft-providers CI doesn't have lxd, mock it out * chore(style): rename per code review * chore(style): rename per code review * chore(style): rename per code review * chore: update changed names in test * refactor: remove globals, let LXDInstances keep project_name * chore: autoformat * fix: compat tag structure had been dependent on craft-application - This way is also simpler in the code, with a slight performance hit for the regex. - There's no way to get the full compat tag without having a fully-instantiated application. - Also fixed some debug output. * feat(tests): add configure hook integration test * feat: added remove hook test and beefed up configure hook test * chore: autoformat * fix: unit tests for changed interface * chore: autoformat, this time with the other tool! * chore: autoformat the third * fix: use the superior is-installed check from c-prov proper * fix: lint * feat: mknod intercepts now configurable from LXDProvider (canonical#717) * feat: mknod intercepts now configurable from LXDProvider * fix: unit test * fix: lint * docs: update changelog (also includes item from canonical#710) * feat: add modify_file method to Executor class * feat: add properties for getting setting pro services applied to environment * feat: make pull optional in Executor.modify_file * fix(LXDInstance): missing pro_services file returns none * trigger ci * refactor: adjustments to satisfy linters --------- Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com> Co-authored-by: Callahan Kovacs <callahan.kovacs@canonical.com> Co-authored-by: Matt Culler <matt.culler@canonical.com>

mattculler self-assigned this Jan 7, 2025

mattculler linked an issue Jan 7, 2025 that may be closed by this pull request

Upstream snap hooks #709

Closed

mattculler force-pushed the work/CRAFT-3821-upstreamed-snap-hooks branch from 10dde28 to f427dce Compare January 8, 2025 20:46

mattculler marked this pull request as ready for review January 9, 2025 15:27

mattculler requested review from cmatsuoka and mr-cal January 9, 2025 15:29

mr-cal reviewed Jan 9, 2025

View reviewed changes

cmatsuoka requested changes Jan 10, 2025

View reviewed changes

mattculler mentioned this pull request Jan 10, 2025

Deduplicate hookutil #715

Open

mattculler added 17 commits January 14, 2025 16:27

feat: add hookutil from downstream repo

2206d9e

Lightly modified to remove proprietary identifiers, not functional in this state.

feat: make hookutil work generically

86bcf50

chore: disable lint check - may change approach later

25d0d91

feat: add unit tests from downstream

4e5bacc

chore: linter issues

815dd5f

fix: craft-providers CI doesn't have lxd, mock it out

fca7e63

chore(style): rename per code review

5291ade

chore(style): rename per code review

8af4a43

chore(style): rename per code review

37405fc

chore: update changed names in test

83c0639

refactor: remove globals, let LXDInstances keep project_name

09873e6

chore: autoformat

3138eb6

fix: compat tag structure had been dependent on craft-application

a612509

- This way is also simpler in the code, with a slight performance hit for the regex. - There's no way to get the full compat tag without having a fully-instantiated application. - Also fixed some debug output.

feat(tests): add configure hook integration test

6af2f21

feat: added remove hook test and beefed up configure hook test

2262b0b

chore: autoformat

e1bab5f

fix: unit tests for changed interface

077854f

mattculler force-pushed the work/CRAFT-3821-upstreamed-snap-hooks branch from 42c5ae1 to 077854f Compare January 14, 2025 21:27

chore: autoformat, this time with the other tool!

f567b84

chore: autoformat the third

d66fc2d

mattculler requested review from cmatsuoka and mr-cal January 15, 2025 06:04

mattculler added the squash label Jan 15, 2025

mr-cal approved these changes Jan 15, 2025

View reviewed changes

fix: use the superior is-installed check from c-prov proper

7fd4e24

cmatsuoka approved these changes Jan 16, 2025

View reviewed changes

fix: lint

24e1fa2

mattculler merged commit 5bbaf39 into main Jan 16, 2025
14 checks passed

mattculler deleted the work/CRAFT-3821-upstreamed-snap-hooks branch January 16, 2025 17:43

mattculler added a commit that referenced this pull request Jan 16, 2025

docs: update changelog (also includes item from #710)

2da89ca

mattculler added a commit that referenced this pull request Jan 16, 2025

docs: update changelog (also includes item from #710)

65e8706

mattculler added a commit that referenced this pull request Jan 16, 2025

feat: mknod intercepts now configurable from LXDProvider (#717)

6ceb498

* feat: mknod intercepts now configurable from LXDProvider * fix: unit test * fix: lint * docs: update changelog (also includes item from #710)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: upstream snap hooks#710

feat: upstream snap hooks#710
mattculler merged 21 commits intomainfrom
work/CRAFT-3821-upstreamed-snap-hooks

mattculler commented Jan 7, 2025 •

edited

Loading

Uh oh!

mr-cal left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmatsuoka left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattculler commented Jan 10, 2025 •

edited

Loading

Uh oh!

mattculler commented Jan 10, 2025

Uh oh!

mr-cal left a comment

Uh oh!

cmatsuoka left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mattculler commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mr-cal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmatsuoka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattculler commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattculler commented Jan 10, 2025

Uh oh!

mr-cal left a comment

Choose a reason for hiding this comment

Uh oh!

cmatsuoka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattculler commented Jan 7, 2025 •

edited

Loading

mattculler commented Jan 10, 2025 •

edited

Loading