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

add valuesOverwrite for image in gvisor #656

Closed
wants to merge 1 commit into from
Closed

add valuesOverwrite for image in gvisor #656

wants to merge 1 commit into from

Conversation

dergeberl
Copy link
Contributor

What this PR does / why we need it:

Add availability to overwrite settings for imageVectorOverwrite in gvisor.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Release note:

NONE

@dergeberl dergeberl requested a review from a team as a code owner November 24, 2021 13:03
@gardener-robot
Copy link

@dergeberl Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Nov 24, 2021
@Diaphteiros
Copy link
Contributor

The image should already be configurable via landscape.versions.gardener.extensions.runtime-gvisor (and then image_tag and/or image_repo). In addition to that, this line https://github.com/gardener/garden-setup/blob/master/components/gardener/extensions/deployment.yaml#L471 does allow a generic overwrite of the values file for the extension helm chart. To overwrite, add valueOverwrites (not valuesOverwrite) to the aforementioned path in the acre.yaml and specify your new values there.

So, I think that the line you added is actually not needed. Please have a look at my comments and if you agree, close the PR. If you don't agree, please explain which case you are trying to solve that is not covered by the existing mechanisms.

@dergeberl
Copy link
Contributor Author

dergeberl commented Nov 24, 2021

I can't inject settings for imageVectorOverwrite in my case:
Example:

runtime-gvisor:
  valueOverwrites:
    test: test 
    image:
      imageVectorOverwrite: |
        images:
        - name: runtime-gvisor-installation
          sourceRepository: github.com/gardener/gardener-extension-runtime-gvisor
          repository: eu.gcr.io/gardener-project/gardener/extensions/runtime-gvisor-installation

Without the the change in this PR only the test: test is injected but image.imageVectorOverwrite is ignored. I think this is because I did not overwrite repositoryand tag.

@Diaphteiros
Copy link
Contributor

Hm, yeah, the way spiff templating works, it could indeed be that you can't add any new fields to the image node. But is that needed? Instead of injecting the imageVectorOverwrite, just put the repository part of it under landscape.versions.gardener.extensions.runtime-gvisor.image_repo and optionally adapt image_tag as well.

So, the functionality that you want is there, it just uses a different syntax than what you would like to use. If you think your syntax has advantages, we can add the line which you proposed, but then I'd prefer that we add it to all extensions, so they all behave the same way. WDYT?

@dergeberl
Copy link
Contributor Author

Hmm, I am not sure. In my opinion it should be possible to use the valueOverwrites like it is documented (https://github.com/gardener/garden-setup/blob/master/docs/extended/gardener.md#valueoverwrites-for-extensions) to overwrite values. Maybe it is better to add the possibility to all extensions, like in this PR. But I guess it is a point for all settings, for example resources https://github.com/gardener/garden-setup/blob/master/components/gardener/extensions/deployment.yaml#L476 . What do you think?

@Diaphteiros
Copy link
Contributor

@dergeberl Sorry, I forgot to follow up on this. The problem with "deep merges" is that they are not supported by the templating language and therefore we would need lots of merge annotations in the templates, which is annoying and hard to read. I'll try to find out whether we can support this kind of merge, then this would be a lot easier and cleaner.

@gardener-robot gardener-robot added the lifecycle/stale Nobody worked on this for 6 months (will further age) label Dec 22, 2022
@gardener-robot gardener-robot added lifecycle/rotten Nobody worked on this for 12 months (final aging stage) and removed lifecycle/stale Nobody worked on this for 6 months (will further age) labels Sep 1, 2023
@dergeberl
Copy link
Contributor Author

This is quite old and also not relevant for us anymore.

@dergeberl dergeberl closed this Apr 5, 2024
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Nobody worked on this for 12 months (final aging stage) needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants