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

Rules in ConfigData can only replace existing nodes in target file, can't add new ones. #71

Closed
dee0 opened this issue Feb 21, 2024 · 6 comments
Assignees
Labels

Comments

@dee0
Copy link

dee0 commented Feb 21, 2024

This is related to #72

See the comment there about the case where no reasonable default is possible. #72 combined with this make it difficult to manage a large number of config values, particularly when they are organized into a structure.

The hack that I came up with is

  • In ConfigData defaults have an empty target node and a 'defaults' node that has a template provides the actual defaults and that is heavily decorated with the ( &default ) marker.
  • Have a rule with the value `(( merge( target, *defaults ) ))

config.zip

@Skarlso
Copy link
Contributor

Skarlso commented Apr 30, 2024

We already have an existing patch merge strategy option to handle large and complex cases for configuration.

@Skarlso
Copy link
Contributor

Skarlso commented Apr 30, 2024

Instead of configRef define a patchStrategicMergeRef that points to an exchange of complex values with a target and a path. The result will be merged based on kustomize merging strategy.

@Skarlso Skarlso self-assigned this May 2, 2024
@Skarlso Skarlso moved this from 🆕 ToDo to 🔍 Review in OCM Backlog Board May 2, 2024
@dee0sap
Copy link

dee0sap commented May 15, 2024

Believe this case has been fixed with the switch to using yqlib for merging the defaults and the config values.
That said it would be best to add a unit test that confirms it prior to closing this.

@Skarlso Skarlso moved this from 🔍 Review to 🏗 In Progress in OCM Backlog Board May 22, 2024
@Skarlso
Copy link
Contributor

Skarlso commented May 22, 2024

I'll add unit tests for these cases.

@Skarlso
Copy link
Contributor

Skarlso commented May 23, 2024

This PR now includes tests for these scenarios because without ocm v0.10.0 these don't work. open-component-model/ocm-controller#425

@Skarlso Skarlso moved this from 🏗 In Progress to 🔍 Review in OCM Backlog Board May 23, 2024
@Skarlso
Copy link
Contributor

Skarlso commented Jun 12, 2024

This should now have been fixed.

@Skarlso Skarlso closed this as completed Jun 12, 2024
@github-project-automation github-project-automation bot moved this from 🔍 Review to 🍺 Done in OCM Backlog Board Jun 12, 2024
@Skarlso Skarlso moved this from 🍺 Done to 🔒Closed in OCM Backlog Board Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🔒Closed
Development

No branches or pull requests

4 participants