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.

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

feat!(ProjectService): allow the ProjectService to work with Imagecraft #674

Merged
merged 15 commits into from
Mar 7, 2025

Conversation

lengau
Copy link
Contributor

@lengau lengau commented Mar 5, 2025

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint && make test?
  • Have you added an entry to the changelog (docs/reference/changelog.rst)?

@lengau lengau force-pushed the work/imagecraft-partitions branch 2 times, most recently from c6ad126 to da33cbf Compare March 5, 2025 23:14
@lengau lengau requested review from upils and mr-cal March 5, 2025 23:38
@lengau lengau requested a review from upils March 6, 2025 15:25
@lengau lengau force-pushed the work/imagecraft-partitions branch from 68b56fd to 0a3a25a Compare March 6, 2025 18:02
@lengau lengau mentioned this pull request Mar 7, 2025
4 tasks
Copy link

@upils upils left a comment

Choose a reason for hiding this comment

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

Thanks! I have mostly questions and suggestions but probably nothing really blocking.
Tests are failing though so I think it should be fixed before merging.

@@ -239,7 +239,7 @@ def _init_lifecycle_manager(self) -> LifecycleManager:
project_vars_part_name=self._project.adopt_info,
project_vars=self._project_vars,
track_stage_packages=True,
partitions=self._services.get("project").get_partitions(),
partitions=self._services.get("project").partitions,
Copy link

Choose a reason for hiding this comment

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

nitpick: using self._services.get(Literal["project"]) is helping the typing interpreter (at least in my IDE) to understand the type of the returned object (and so to suggest methods and fields). That is a bit uglier though so maybe we should define a constants for the available services provided by craft-application?

PROJECT_SERVICE = Literal["project"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which IDE are you using that's doing this? I'm not seeing this issue in pycharm or using the pyright LSP.

assert project_data == expected


@pytest.mark.parametrize(
"build_for", [arch.value for arch in craft_platforms.DebianArchitecture] + ["all"]
"build_for", [arch.value for arch in craft_platforms.DebianArchitecture]
)
@pytest.mark.usefixtures("enable_partitions")
def test_expand_environment_stage_dirs(
Copy link

@upils upils Mar 7, 2025

Choose a reason for hiding this comment

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

note: I am confused about this test. I failed to understand why it is using the platform/build_for values as partition names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's to test that we have variable/flexible partitions based on the platform. We're using the FakeProjectService from conftest.py here which does that. I've added a comment to explain.

Copy link
Contributor

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Still looks good to me!

@lengau lengau merged commit 395f9c8 into feature/hybrid-commands Mar 7, 2025
13 of 17 checks passed
@lengau lengau deleted the work/imagecraft-partitions branch March 7, 2025 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants