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(lifecycle): add deprecated snap command #4589

Merged
merged 4 commits into from Feb 23, 2024
Merged

feat(lifecycle): add deprecated snap command #4589

merged 4 commits into from Feb 23, 2024

Conversation

syu-w
Copy link
Contributor

@syu-w syu-w commented Feb 16, 2024

No description provided.

@codecov Codecov
Copy link

codecov bot commented Feb 16, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.90%. Comparing base (6ff3e0b) to head (52c805b).
Report is 13 commits behind head on feature/craft-application.

Additional details and impacted files 
@@                      Coverage Diff                      @@
##           feature/craft-application    #4589      +/-   ##
=============================================================
+ Coverage                      88.83%   88.90%   +0.06%     
=============================================================
  Files                            327      332       +5     
  Lines                          22033    22116      +83     
=============================================================
+ Hits                           19573    19662      +89     
+ Misses                          2460     2454       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sergiusens
sergiusens reviewed Feb 20, 2024
View reviewed changes
tests/conftest.py Outdated Show resolved Hide resolved
sergiusens
sergiusens reviewed Feb 20, 2024
View reviewed changes
tests/conftest.py Outdated Show resolved Hide resolved
sergiusens
sergiusens requested changes Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@sergiusens sergiusens 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 overall, but some changes have been a bit too overreaching

@syu-w syu-w force-pushed the work/CRAFT-2479-deprecate-snap-command branch from c978fe1 to 2ae71d1 Compare February 20, 2024 17:18
tigarmo
tigarmo reviewed Feb 20, 2024
View reviewed changes
snapcraft/commands/lifecycle.py Show resolved Hide resolved
tests/conftest.py Outdated
arch = util.get_host_architecture()
return [
models.BuildInfo(
"ubuntu-22.04", arch, arch, bases.BaseName("ubuntu", "22.04")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why these test fixtures are being copied from craft-application, but I think these bases should specify 24.04 at least

tigarmo
tigarmo approved these changes Feb 20, 2024
View reviewed changes
snapcraft/commands/lifecycle.py Show resolved Hide resolved
@sergiusens
Copy link
Collaborator

I am still not seeing why we cannot use the fixtures like we had from Rockcraft and we we had to bring in the Craft Application ones

@tigarmo
Copy link
Contributor

tigarmo commented Feb 21, 2024

I am still not seeing why we cannot use the fixtures like we had from Rockcraft and we we had to bring in the Craft Application ones

I agree actually. We don't need to bring in everything wholesale, we can put in just the things needed for the new tests. Simpler and less code to maintain.

@syu-w syu-w force-pushed the work/CRAFT-2479-deprecate-snap-command branch from 5624231 to 7777f02 Compare February 21, 2024 20:32
@syu-w
Copy link
Contributor Author

syu-w commented Feb 21, 2024

This should significantly simplified the fixtures.

(I had to force push due to merged wrong branch)

sergiusens
sergiusens reviewed Feb 21, 2024
View reviewed changes
tests/conftest.py Outdated
Comment on lines 81 to 95
def features(request) -> dict[str, bool]:
"""Fixture that controls the enabled features.

To use it, mark the test with the features that should be enabled. For example:

@pytest.mark.enable_features("build_secrets")
def test_with_build_secrets(...)
"""
features = {}

for feature_marker in request.node.iter_markers("enable_features"):
for feature_name in feature_marker.args:
features[feature_name] = True

return features
Copy link
Collaborator

Choose a reason for hiding this comment

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

we do not use any features in Snapcraft, so why have this?

@sergiusens
Copy link
Collaborator

to set things straight, I would say, rebase and drop 4f492aa and work on extending the fixtures we have with the things we need

@syu-w syu-w force-pushed the work/CRAFT-2479-deprecate-snap-command branch from 7777f02 to 6e25c37 Compare February 21, 2024 23:12
@sergiusens
Copy link
Collaborator

@syu-w you did not seem to extend anything still, you moved the fixtures from tests/unit/conftest.py to tests/conftests.py. What is the justification for this?

@syu-w
Copy link
Contributor Author

syu-w commented Feb 21, 2024

@syu-w you did not seem to extend anything still, you moved the fixtures from tests/unit/conftest.py to tests/conftests.py. What is the justification for this?

The snap command test only need to add the fake_services which using the 2 exist fixtures in the tests/unit/conftest.py. And it does not need to extend more fixtures for it to work.

@syu-w syu-w force-pushed the work/CRAFT-2479-deprecate-snap-command branch from 6e25c37 to 5463ba7 Compare February 22, 2024 00:47
@syu-w syu-w force-pushed the work/CRAFT-2479-deprecate-snap-command branch from 5463ba7 to 564136e Compare February 22, 2024 20:17
@syu-w
Copy link
Contributor Author

syu-w commented Feb 22, 2024

Rebase with no changes.

@tigarmo
Copy link
Contributor

tigarmo commented Feb 22, 2024

@syu-w not sure if you are aware but there are a lot of new spread test failures

2024-02-22 22:35:04 Error preparing google:ubuntu-22.04-64:tests/spread/general/verbosity (feb222220-944854) : 
-----
+ snapcraft init
Traceback (most recent call last):
  File "/snap/snapcraft/x1/bin/snapcraft", line 8, in <module>
    sys.exit(main())
  File "/snap/snapcraft/x1/lib/python3.10/site-packages/snapcraft/application.py", line 342, in main
    return app.run()
  File "/snap/snapcraft/x1/lib/python3.10/site-packages/craft_application/application.py", line 451, in run
    dispatcher = self._get_dispatcher()
  File "/snap/snapcraft/x1/lib/python3.10/site-packages/snapcraft/application.py", line 184, in _get_dispatcher
    dispatcher = craft_cli.Dispatcher(
  File "/snap/snapcraft/x1/lib/python3.10/site-packages/craft_cli/dispatcher.py", line 230, in __init__
    self.commands = _get_commands_info(commands_groups)
  File "/snap/snapcraft/x1/lib/python3.10/site-packages/craft_cli/dispatcher.py", line 193, in _get_commands_info
    raise RuntimeError(
RuntimeError: Multiple commands with same name: Snap and SnapCommand
-----

@sergiusens sergiusens merged commit fa855f3 into feature/craft-application Feb 23, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergiusens sergiusens sergiusens approved these changes

@tigarmo tigarmo tigarmo approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@syu-w @sergiusens @tigarmo