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

support multiple configurations as an object not a single string #151

Merged
merged 3 commits into from
Mar 14, 2023

Conversation

hafizmujadidKhalid
Copy link
Contributor

Hello,
I was trying to add some additional configurations such as below:

retry-policy=TASK
query.hash-partition-count=50

But it adds both properties in a single line as extraConfig: "" is a string value. We should make it an object and store all extra configuration in config.properties as it is given by the user.

This PR fixes issue #150

@hafizmujadidKhalid
Copy link
Contributor Author

@valeriano-manassero!
Can you please have a look on this PR? helm-docs generation worked fine on my local but it fails on github. Error is also not explainatory to know what is going wrong actually.

@valeriano-manassero
Copy link
Owner

Pls generate README with helm-docs and commit it.

@hafizmujadidKhalid
Copy link
Contributor Author

Thanks, Looks good now.

@valeriano-manassero
Copy link
Owner

I'd like to see version 3.0.0 instead of 2.8.2. This is because we are in front of a potential breaking change (especially if someone already is using extraConfig).
Can you pls fix it?

@valeriano-manassero valeriano-manassero merged commit 337af89 into valeriano-manassero:main Mar 14, 2023
@cccs-nik
Copy link
Contributor

@hafizmujadidKhalid Sounds like you were getting a single line string because you don't know how to use multi-line strings? Please refer to this documentation: https://helm.sh/docs/chart_template_guide/yaml_techniques/#strings-in-yaml. I've only ever seen multi-line strings used to populate data into ConfigMaps for something like this.

While this change "works" (although not implemented properly: coalesce.go:199: warning: cannot overwrite table with non table for extraConfig (map[])) it is absolutely awful in practice.

Would it please be possible to revert this change? @valeriano-manassero Thanks.

@valeriano-manassero
Copy link
Owner

valeriano-manassero commented Mar 23, 2023

Would it please be possible to revert this change? @valeriano-manassero Thanks.

I will do another major release with rollback. Sorry I accepted the PR without having you as reviewer. Maybe I can add you next time.

@cccs-nik
Copy link
Contributor

Yeah no worries, I can help with reviews. Feel free to add me on future PRs.

valeriano-manassero added a commit that referenced this pull request Mar 24, 2023
valeriano-manassero added a commit that referenced this pull request Mar 24, 2023
* Revert "support multiple configurations as an object not a single string (#151)"

This reverts commit 337af89.

* Changed: bump up version
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.

3 participants