-
Notifications
You must be signed in to change notification settings - Fork 117
Extend the defaults accepted via configuration #2476
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
base: master
Are you sure you want to change the base?
Extend the defaults accepted via configuration #2476
Conversation
|
Thank you @benedikt-haug for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
|
@benedikt-haug Thank you for your contribution. |
|
/ok-to-test |
|
Nice! |
|
Maybe it would be better to group the default options under a key in the config, e.g., Additionally, these values need to be added to the Helm charts. Right now, the Helm charts are the only documentation for the dashboard configuration. We should consider introducing a dedicated documentation page for the configuration — but that’s a separate task and not related to this PR. |
Sure <3
Seems reasonable :) Incorporated that feedback. Also moved the "old" variables there, while including a fallback for the users of the old naming scheme so things don't suddenly break.
Added an example like the other repos have it. Also tried to extend the helm charts to feature the new variable. Do you know how to test the charts locally, |
We implemented tests for the help charts. You can adapt them and run them with |
|
@holgerkoser, @klocke-io You have pull request review open invite, please check |
though my linter is persistent about using shootDefaults.value instead
22cfabb to
fac17ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @grolu,
thanks for having a look :)
I think I've successfully pulled off the rebase :D
Added the configStore to the cloudProfile.
You are correct about my changes being a breaking change. Is announcing this something I'd do in this merge request or something that is written somewhere else?
I personally don't need to change the infrastructure list. I just wanted to move all hardcoded defaults to configstore. I thought that maybe it would be fun to be able to change the sorting to prioritize specific infrastructures or something.
It is sufficient to mention in the PR description with
Right, exactly this can now be achieved using weights in the vendor configuration. So I think we can drop the infra defaults in this PR |
|
@grolu |
|
Okay this looks good. I also like that you added configuration examples to the values.yaml. |
|
Hi @benedikt-haug |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What this PR does / why we need it:
This PR is part of the Hackaton.
This PR extend the defaults gardener dashboard accepts via configuration.
Additionally, it moves the default declaration to the config store so they are declared in the same spot.
Lastly, it also adds the example folder describing the new additions made possible by the gardener-dashboard-frontend configmap.
Which issue(s) this PR fixes:
Special notes for your reviewer:
/cc @marc1404 @klocke-io
Release note: