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

Merge craft-application update into main #527

Merged
merged 6 commits into from
Apr 5, 2024
Merged

Conversation

tigarmo
Copy link
Collaborator

@tigarmo tigarmo commented Apr 5, 2024

The update to the latest craft-application update was done over time in this feature/craft-application branch, which is now ready to be integrated back into main. Only the latest commit needs to be reviewed because we might as well update to the very new 2.4.0 - all other commits have already been reviewed in various past PRs.

@tigarmo tigarmo changed the title Feature/craft application Merge craft-application update into main Apr 5, 2024
@tigarmo tigarmo marked this pull request as ready for review April 5, 2024 12:59
@cmatsuoka
Copy link
Contributor

Only the latest commit needs to be reviewed

Is this the one that updates craft-application to 2.4.0?

@tigarmo
Copy link
Collaborator Author

tigarmo commented Apr 5, 2024

Only the latest commit needs to be reviewed

Is this the one that updates craft-application to 2.4.0?

yes

Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

LGTM. Also moving build-base settings from the extension to user if the extension is set and base is bare sounds like a sensible decision in the long term.

tigarmo added 6 commits April 5, 2024 14:39
The BuildPlanner has the project's base, build-base and platform declarations,
and does all the validation related to those fields.
This cleans-up the code related to functionality that is now implemented
upstream. This includes:

- registering application plugins;
- package-repositories installation (including overlays).

It also updates some tests to latest features, like "parallel_build_count".
The brunt of the update is updating usages of platform/build-for to the build
plan. This largely affects the image and package services, but the semantics
should be unchanged because it's the same data just delivered differently.
The problem is this: the flask extension currently adds a 'build-base' if the
'base' is 'bare' but the 'build-base' is undeclared, and a 'platforms' entry
if it's missing,  but this behavior breaks with the new version of
craft-application because the BuildPlanner processes the yaml _before_ the
extensions are applied, and the planner cannot work without a build-base or
platform description.

The extension behavior of adding a 'build-base' and a 'platforms' item needs
to be reviewed: we probably don't want to support this. In the meantime,
this commit fixes only the projects that are executed in spread.
@lengau lengau force-pushed the feature/craft-application branch from 17beef2 to 3c0c48d Compare April 5, 2024 18:39
@lengau
Copy link
Contributor

lengau commented Apr 5, 2024

Rebased on main, should be ready to rebase-merge when CI completes

@tigarmo tigarmo merged commit 3690f7b into main Apr 5, 2024
13 checks passed
@tigarmo tigarmo deleted the feature/craft-application branch April 5, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants