search

canonical / craft-application

The basis for *craft applications
https://canonical-craft-application.readthedocs-hosted.com/en/latest
GNU Lesser General Public License v3.0
5 stars 6 forks source link

fix: apply yaml transforms before grammar #316

Closed mr-cal closed 2 months ago

mr-cal commented 2 months ago

extra_yaml_transforms should be applied before grammar because an application may add advanced grammar.

I'm targeting hotfix/2.5 so this can be released as craft-application 2.5.1 and rolled out to snapcraft 8.2.1.

See snapcraft integration here: https://github.com/canonical/snapcraft/pull/4753

LP#2061603 (CRAFT-2817)

tigarmo commented 2 months ago

@mr-cal I think we might want to target main with this instead. I suppose it's debatable whether this is a bugfix or a new feature, but my reasoning is more practical: I have a task to do (support key assets) for the same snapcraft release and that's going to be a minor version bump anyway, so might as well put it all on main. @lengau what do you think?

mr-cal commented 2 months ago

@mr-cal I think we might want to target main with this instead. I suppose it's debatable whether this is a bugfix or a new feature, but my reasoning is more practical: I have a task to do (support key assets) for the same snapcraft release and that's going to be a minor version bump anyway, so might as well put it all on main. @lengau what do you think?

Here's what I've been thinking:

If we target hotfix/2.5 (or a hotfix/2.6 to accomdate your PR) it gives me some breathing room to re-work the breaking changes from #302 before it is included in a release.

During Friday's planning, we talked about implementing a default BuildPlanner in craft-application. I would like to implement a default _providers_base() function as well. This would let me revert the breaking change in #302 where applications are required to create their own Project subclass and implement the abstract _providers_base() function.

lengau commented 2 months ago

@tigarmo I'm going to guess that you'll be the one doing the release next week so whatever you say goes.

tigarmo commented 2 months ago

@mr-cal I think I'm fine with doing a hotfix/2.6 then, particularly because you'll want #318 in too right? I'd say both #318 and #317 are new features so hotfix/2.5 is not really appropriate imo. The other option would be to revert #302 from main until we/you have a chance to do the extra work that would satisfy you for a 3.0 release. This would let us keep merging things into main in the meantime but I'm fine with either option.

mr-cal commented 2 months ago

@tigarmo - let's plan to land this PR and your PR (#317) on a hotfix/2.6 branch.

318 is not an easy egg to crack and I can get 95% of SNAPCRAFT_ and CRAFT_ variables working with changes only in Snapcraft, so we can deal with #318 and the breaking changes in #302 after Thursday.

tigarmo commented 2 months ago

Sounds good to me, ship it!

On Tue, Apr 23, 2024, 19:09 Callahan @.***> wrote:

@tigarmo https://github.com/tigarmo - let's plan to land this PR and your PR (#317 https://github.com/canonical/craft-application/pull/317) on a hotfix/2.6 branch.

318 https://github.com/canonical/craft-application/pull/318 is not an

easy egg to crack and I can get 95% of SNAPCRAFT and CRAFT variables working with changes only in Snapcraft, so we can deal with #318 https://github.com/canonical/craft-application/pull/318 and the breaking changes in #302 https://github.com/canonical/craft-application/pull/302 after Thursday.

— Reply to this email directly, view it on GitHub https://github.com/canonical/craft-application/pull/316#issuecomment-2073556207, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO4MQYIRACLFBI4NRELPKDY63L2XAVCNFSM6AAAAABGTQXMDWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZTGU2TMMRQG4 . You are receiving this because you were mentioned.Message ID: @.***>