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

examples: fix up #64

Closed
wants to merge 4 commits into from
Closed

examples: fix up #64

wants to merge 4 commits into from

Conversation

supakeen
Copy link
Member

@supakeen supakeen commented May 8, 2024

The examples merged in #52 were broken (and I merged them knowing this) so now let's work to make the implementation support them.

Relevant bugs:

@supakeen
Copy link
Member Author

supakeen commented May 8, 2024

Note that with the current commits this is still broken. This is due to the interaction between merging maps, includes, and being inside a define block. Very fun!

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

I am not sure that the goal at this stage should be to have an implementation for our spec. I think the goal should be to create the best possible yaml via discussions and use-cases and then think about the implementation. I worry we make compromises too early otherwise (but happy to hear input from others here of course, every team does things differently).


otk.include: "minimal-common.yaml"
otk.target.osbuild:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can do better here, not including the pipelines from something like "common.yaml" will mean we have quite a bit of duplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could resolve includes before omnifest validation.

@@ -1,7 +1,14 @@
otk.version: 1

otk.define:
version: 40
architecture: "aarch64"
otk.op.join:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change driven by the current implementation or is this the yaml we want? I'm asking because it does not feel like an improvement and I think at this stage of the project we should aim for the best possible yaml and then think about the implementation (and being able to have a toplevel with otk.define and an include that also otk.defines stuff without the need for an explicit otk.op.join seems nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is driven by the current implementation and what is possible in it. And yes; the 'way it is' (in main) is nicer :)

It should test the directive directly instead of going through the
omnifset parsing as well.

Signed-off-by: Simon de Vlieger <[email protected]>
Desugar is currently not correctly replacing multiple variables in a
string. This adds failing test cases for that.

Signed-off-by: Simon de Vlieger <[email protected]>
Use regular expressions to do replacements. Previously we didn't replace
multiple occurences of variables correctly.

Signed-off-by: Simon de Vlieger <[email protected]>
Sadly a lot of the deduplication work that has been done has to be
undone for now due to the way directives work. Let's get main working
then put them back in in feature branches.

Signed-off-by: Simon de Vlieger <[email protected]>
@supakeen
Copy link
Member Author

Going to close this PR and resubmit due to shenanigans going on on GitHub/my local checkout.

@supakeen supakeen closed this May 13, 2024
@supakeen supakeen deleted the fix-up-examples branch May 13, 2024 09:39
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.

2 participants