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: Support default CR starting with dashes #149

Merged
merged 9 commits into from
Jan 31, 2025

Conversation

nesmabadr
Copy link
Contributor

Description

Changes proposed in this pull request:

  • Support Default CR starting with dashes
  • Add unit test for this case

Related issue(s)
Resolves #143

@nesmabadr nesmabadr requested a review from a team as a code owner January 30, 2025 08:44
@kyma-bot kyma-bot added size/L and removed size/M labels Jan 30, 2025
c-pius
c-pius previously approved these changes Jan 31, 2025
Copy link
Contributor

@c-pius c-pius left a comment

Choose a reason for hiding this comment

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

Consider it approved with suggestion :) The change looks good, one suggestion would be if we could consider streamlining the test cases (only if time allows and you feel like doing it)...

It looks as if we have a set of very similar tests where each is having a shared core (the minimal information that is put into the module template) and each test adding a small part on top (e.g., when associated resources are present, when downtime is set, ...) Would it be possible to use the pattern with an array of test cases where we pass assert functions to the test case? We could have one assert function that is used by all tests verifying the common part, and then pass one function that verifies the specific addition by this test. This would make it easier to see the common part, the individual changes by each test, and make it more robust if things change in the common part in the future.

@nesmabadr nesmabadr force-pushed the remove_yaml_separators branch from e9a0899 to d0d0098 Compare January 31, 2025 10:03
Copy link
Contributor

@c-pius c-pius left a comment

Choose a reason for hiding this comment

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

Nice!

@kyma-bot kyma-bot added the lgtm label Jan 31, 2025
@nesmabadr nesmabadr merged commit 1f95f05 into kyma-project:main Jan 31, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check if we parse yamls correctly
3 participants