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

fix: apply yaml transforms before grammar #316

Closed
wants to merge 1 commit into from
Closed

fix: apply yaml transforms before grammar #316

wants to merge 1 commit into from

Conversation

mr-cal
Copy link
Contributor

@mr-cal mr-cal commented Apr 22, 2024 •
edited
Loading

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

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: canonical/snapcraft#4753

LP#2061603
(CRAFT-2817)

@mr-cal
fix: apply yaml transforms before grammar
77a691e 
`extra_yaml_transforms` should be applied before grammar because an application
may add advanced grammar.

Signed-off-by: Callahan Kovacs <[email protected]>
@mr-cal mr-cal force-pushed the work/CRAFT-2817-transform-before-grammar branch from d11f105 to 77a691e Compare April 23, 2024 13:58
@mr-cal mr-cal changed the base branch from main to hotfix/2.5 April 23, 2024 13:58
@mr-cal mr-cal requested review from tigarmo and lengau April 23, 2024 14:08
tigarmo
tigarmo approved these changes Apr 23, 2024
View reviewed changes
tests/unit/test_application.py
return new_yaml_data


def test_process_grammar_from_extra_transform(app_metadata, fake_services, tmp_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@tigarmo
Copy link
Contributor

tigarmo commented Apr 23, 2024

@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
Copy link
Contributor Author

mr-cal commented Apr 23, 2024 •
edited
Loading

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

  • feat!: validate devel bases #302 landed on main after 2.5.0 and is a backward incompatible change that technically should be a craft-application 3.0 release
  • We need to get a few other craft-application fixes released by Thursday for snapcraft
  • I want to do a partial revert of the breaking changes in feat!: validate devel bases #302 (details in the last paragraph)

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
Copy link
Contributor

lengau commented Apr 23, 2024

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

@tigarmo
Copy link
Contributor

tigarmo commented Apr 23, 2024

@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
Copy link
Contributor Author

mr-cal commented Apr 23, 2024 •
edited
Loading

@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
Copy link
Contributor

tigarmo commented Apr 23, 2024 via email

@mr-cal mr-cal deleted the branch hotfix/2.5 April 23, 2024 22:19
@mr-cal mr-cal closed this Apr 23, 2024
@mr-cal mr-cal mentioned this pull request Apr 23, 2024
Merged
3 tasks
@mr-cal mr-cal deleted the work/CRAFT-2817-transform-before-grammar branch April 24, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lengau lengau lengau approved these changes

@tigarmo tigarmo tigarmo 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.
3 participants
@mr-cal @tigarmo @lengau