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

Fix ocm issue 179, block in config yields invalid yaml #734

Conversation

dee0sap
Copy link
Contributor

@dee0sap dee0sap commented Apr 20, 2024

Description

Fixes open-component-model/ocm-project#179

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

  • Related Issue # 179
  • Closes # (179)
  • Fixes # (179)

Remove if not applicable

Screenshots

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help
  • Separate ticket for tests # (issue/pr)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

  • πŸ“œ README.md
  • πŸ™… no documentation needed

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@dee0sap dee0sap changed the title Add test that shoes ocm issue 179, block in config yields invalid xml Add test that shows ocm issue 179, block in config yields invalid xml Apr 20, 2024
@morri-son
Copy link
Contributor

@mandelsoft , can you please takle a look at this proposal?

@Skarlso Skarlso force-pushed the ocm-issue-179-block-in-config-yields-invalid-xml branch from 819ebf5 to 527d458 Compare April 25, 2024 10:45
@Skarlso
Copy link
Contributor

Skarlso commented Apr 25, 2024

@dee0sap All you have to do is indent the value that you wish to substitute correctly.

We marshal values before doing anything this PR now fixes that in the sense that we don't marshal primitive values. That marshalling broke the syntax of the value that we are trying to insert. So we leave values alone. But we do marshal complex types like struct, pointer, maps, etc.

@Skarlso Skarlso changed the title Add test that shows ocm issue 179, block in config yields invalid xml Add test that shows ocm issue 179, block in config yields invalid yaml Apr 25, 2024
- Reduce test fragility by using MatchYAML and MatchJSON instead of checking for string equality which breaks with minor white space changes
- Remove unused Set function which exposes implementation detail of goccy usage
…-model#179.   Confirmed that pre-yqlib the test fails and that after yqlib it passes.
…e substitution data is converted to yaml format
@dee0sap dee0sap changed the title Add test that shows ocm issue 179, block in config yields invalid yaml Fix ocm issue 179, block in config yields invalid yaml May 1, 2024
@dee0sap
Copy link
Contributor Author

dee0sap commented May 1, 2024

I think this PR is ready for whatever review process you guys have.

Copy link
Contributor

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

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

LGTM

@Skarlso Skarlso merged commit ea79db6 into open-component-model:main May 3, 2024
12 checks passed
@Skarlso
Copy link
Contributor

Skarlso commented May 3, 2024

If anything breaks because of this, we'll iterate on a solution. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🍺 Done
Development

Successfully merging this pull request may close these issues.

Configuration generates invalid yaml for multiline and object content
3 participants