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

Local values are not taken at all if global is set #536

Closed
michal-jagiello-tmpl opened this issue Jun 6, 2023 · 11 comments
Closed

Local values are not taken at all if global is set #536

michal-jagiello-tmpl opened this issue Jun 6, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@michal-jagiello-tmpl
Copy link
Contributor

Describe the bug
With 6f69e00 commit I observed regression in our testkube instance. We are using only local registries on our clusters (as we don't have an internet access there) and for each kind of registry (docker.io, gcr.io) we have separate url. As docker.io is the most common one we used that value as a global one and for each other registry we overwritten them using "local" variables. As far as I understand helm values local ones should overwrite global ones, but right now it's not available. We observed that on operator-pre-upgrade job.

To Reproduce
Steps to reproduce the behavior:

  1. Run helm template . --set="global.imageRegistry=test"
  2. Check that all images uses "test" registry

Expected behavior
If local variables are set the default/global variable should be overwritten

Version / Cluster

  • Which testkube version? 1.12.9
@michal-jagiello-tmpl
Copy link
Contributor Author

Based on discussion from helm/helm#5676 it's expected that local overwrites global ones

@vsukhin vsukhin added the enhancement New feature or request label Jun 6, 2023
@vsukhin
Copy link
Contributor

vsukhin commented Jun 6, 2023

thank you @michal-jagiello-tmpl we will check it out

@ypoplavs
Copy link
Contributor

Hi @michal-jagiello-tmpl,

Thanks for using Testkube :)

Overriding all local values by setting a global one is working by design. The point was to update the registry in all charts by just changing just one line.
If we try to change this feature now, it will break a functionality for existing users.

Could you please use local variables in your case and leave out global so that it won’t affect the values?

We always try to target the most common scenarios, but sometimes they interfere with each other :(
In any case, we will be glad to add any suggestions for improvements.

@michal-jagiello-tmpl
Copy link
Contributor Author

Hi @ypoplavs Thanks for response.
In that case overwriting values doesn't work, as global is not used at all, what's in opposite with helm assumptions in my opinion and I'm afraid any new integrator would face the same issue as they would prefer to overwrite registry in one place (global) and (if needed) overwrite registry in sub projects but only if it's different value than in globals. Right now we need to set the same value in two places.

You mention that would broke functionality for existing users -- that's exactly what we faced -- that change broke our installations.

It's hard to believe for me that people in most common scenarios assume that global values won't be used, especially in image registry case. But maybe I don't know some corner cases and it'd useful and easier for me to accept that fact, also understand, if you try to explain me that better.

@michal-jagiello-tmpl
Copy link
Contributor Author

@ypoplavs please take a look on my change #537
It allows you to change image registry in one line, but would use global as default and you could overwrite it with local one.
Currently if you have a local one and global global would be used and not a local

@ypoplavs
Copy link
Contributor

Thanks @michal-jagiello-tmpl! I will take a look.

@ypoplavs
Copy link
Contributor

Thank you for taking time and releasing a PR @michal-jagiello-tmpl ! We have tested it and it does override global variables when locals are set. However, my concern is the following: how the users, that use globals as a mechanism for overriding locals, will transition to that change?

One of the reasons we designed it that way was to follow Bitnami standards since we use Bitnami MongoDB in our charts as well: https://github.com/bitnami/charts/blob/main/bitnami/mongodb/values.yaml#L3

@michal-jagiello-tmpl
Copy link
Contributor Author

@ypoplavs sorry for late response. Users have to change helm values files to overwrite variables. So if they used globals change is needed only to switch from globals into local variables

@ypoplavs
Copy link
Contributor

Hi @michal-jagiello-tmpl!
In order to switch to that change, the users would need to prepare their values files, change workflows etc, but unfortunately not everyone is keeping an eye on the latest released updates and might not be informed in time. Therefore, as of now we have decided to stick to the current logic of setting the globals.

Thank you for the request. In case there’s a change we will inform you instantly.

@TheBrunoLopes
Copy link
Member

Hello @michal-jagiello-tmpl we would love to know a bit more about how you structure your deployment process in helm and see how we can assist you best.
Would you like to hop on a quick call with us to go over this issue and your testing workflow ?

Here's a my callendly for your convenience: https://calendly.com/bruno-at-kubeshop/30min-1

@michal-jagiello-tmpl
Copy link
Contributor Author

@TheBrunoLopes thanks for an answer. Ok, I created an appointment at 29th August as I start holidays from monday and today and tomorrow your calendar is full :( but no worries, right now it's not a blocker for us

@ypoplavs ypoplavs closed this as completed Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

4 participants