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: multi-base support for charm build plans #81

Merged
merged 7 commits into from
Dec 13, 2024
Merged

feat: multi-base support for charm build plans #81

merged 7 commits into from
Dec 13, 2024

Conversation

mr-cal
Copy link
Contributor

@mr-cal mr-cal commented Dec 9, 2024 •
edited
Loading

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint && make test?

Adds multi-base support for charm build plans.

Documentation will be added in another PR (#70).

Issues I found into when working on this task:

Fixes #73
(CRAFT-3721) (the branch name has the wrong number)

@mr-cal
feat: multi-base support for charm build plans
d8d3d7b 
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
@mr-cal mr-cal force-pushed the work/CRAFT-3271-multi-base-charms branch from 065e015 to d8d3d7b Compare December 9, 2024 19:32
@mr-cal mr-cal requested review from lengau and cmatsuoka December 9, 2024 21:19
lengau
lengau reviewed Dec 10, 2024
View reviewed changes
craft_platforms/__init__.py Outdated Show resolved Hide resolved
cmatsuoka
cmatsuoka reviewed Dec 10, 2024
View reviewed changes
craft_platforms/_architectures.py Show resolved Hide resolved
craft_platforms/_architectures.py Outdated Show resolved Hide resolved
craft_platforms/_architectures.py Outdated Show resolved Hide resolved
craft_platforms/charm/_build.py Outdated Show resolved Hide resolved
craft_platforms/charm/_build.py Outdated Show resolved Hide resolved
craft_platforms/charm/_build.py Outdated Show resolved Hide resolved
@mr-cal
chore: feedback from PR
4dd7d37 
- rename variables
- code simplifications
- clarify doc strings

Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
@mr-cal
tests: add a test for multiple build-for: all
410a80f 
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
@mr-cal
feat: use CraftPlatformsError, restrict shorthand notation
ffc4132 
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
@mr-cal
Copy link
Contributor Author

mr-cal commented Dec 11, 2024

In addition to the PR feedback, I switched to CraftPlatformsError and tightened the validation for the shorthand notation as we discussed in ST124.

So now, you can't declare the base in the platform name and declare build-on and build-for. This is invalid:

platforms:
  ubuntu@24.04:my-platform:
    build-on: amd64
    build-for: amd64

@mr-cal mr-cal requested review from cmatsuoka and lengau December 11, 2024 22:04
lengau
lengau approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@lengau lengau 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

cmatsuoka
cmatsuoka approved these changes Dec 12, 2024
View reviewed changes
Copy link

@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.

Thank you. This is now easier to understand, but be prepared for a bad score in cyclomatic analysis (which may require a future refactoring).

@mr-cal
Copy link
Contributor Author

mr-cal commented Dec 12, 2024

Thanks for the reviews, I'm going to leave this for another day or two in case I have any changes to make when consuming it in craft-application.

mr-cal and others added 2 commits December 12, 2024 13:23
@mr-cal
Merge branch 'main' into work/CRAFT-3271-multi-base-charms
9a8f74f
@mr-cal
chore: make args positional
b721a20 
Following Claudio's suggestion so we are slightly less tied to the arg names.

Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
@lengau lengau merged commit 519b705 into main Dec 13, 2024
12 of 13 checks passed
@lengau lengau deleted the work/CRAFT-3271-multi-base-charms branch December 13, 2024 20:51
@mr-cal mr-cal mentioned this pull request Dec 17, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmatsuoka cmatsuoka cmatsuoka approved these changes

@lengau lengau lengau approved these changes

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

Successfully merging this pull request may close these issues.

Add multi-bases support for charm build plans
3 participants
@mr-cal @cmatsuoka @lengau