Parameterize images #31
Conversation
|
See this change for how new image versions will need to be updated given this change. |
|
With this change we can then follow up with paramerizing the env vars as kfdef params, and sub them in via kfdef similar to this. |
HumairAK
force-pushed
the
parameterize_images
branch
from
5b02e70
to
794269f
Compare
|
@DharmitD w.r.t to this comment
No, tests have their own default configs that are independent of what's deployed in live environment by default, see changes in |
HumairAK
force-pushed
the
parameterize_images
branch
from
794269f
to
51b14a8
Compare
This change introduces the ability to pass in default images via either a configmap or env vars. Environment variables take precedence over configmaps. Environment variables follow the same nested behavior as the yaml config equivalent. For example "images.apiserver" in a `config.yaml` passed in to the operator is the equivalent to passing in an ENV Variable `IMAGES_APISERVER`.
HumairAK
force-pushed
the
parameterize_images
branch
from
51b14a8
to
776b5f1
Compare
This change updates existing tests to consume DSPO config files as part of it's testing. For test cases that use default settings, the config files are used. For cases where cr specifies specific images, the tests consume the config but verify that it is not used over those specified in the CR.
HumairAK
force-pushed
the
parameterize_images
branch
from
776b5f1
to
4598e8d
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gmfrasca The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -124,7 +124,7 @@ func (p *DSPAParams) SetupDBParams(ctx context.Context, dsp *dspa.DataSciencePip | |||
| if p.MariaDB == nil { | |||
| p.MariaDB = &dspa.MariaDB{ | |||
| Deploy: true, | |||
| Image: config.MariaDBImage, | |||
| Image: config.GetStringConfigWithDefault(config.MariaDBImagePath, config.DefaultImageValue), | |||
There was a problem hiding this comment.
@HumairAK the only thing I didn't understand about this PR is this GetStringConfigWithDefault. It looks like DefaultImageValue is a const set to MustSetInConfig, so there's not really any useful fallback value ever returned from GetStringConfigWithDefault. What is your intent there? Is it to make sure image pulls fail with cannot pull docker/MustSetInConfig ? Is that even what would happen?
Perhaps a comment on GetStringConfigWithDefault would help.
There was a problem hiding this comment.
@gregsheremeta you're right the default here will never be used, it was mostly to just stay consistent with the rest of the pattern and add additional future proof protection to the logic.
as you already know, this would never happen because we enforce the requirement of the Image config.
however to me this seemed a bit too decoupled from the rest of the params logic (relying on configs/env vars), so it made sense to have this additional check. Otherwise we are relying on the config file or env variables to be set correctly, are not empty, etc.
Having said that, I'm not against just setting the image directly without using GetStringConfigWithDefault.
There was a problem hiding this comment.
to just stay consistent with the rest of the pattern
which pattern do you mean?
add additional future proof protection to the logic
what's an example of a future change that this is protecting against?
Otherwise we are relying on the config file or env variables to be set correctly, are not empty, etc.
that makes sense, but I think you could also just panic about it with a direct message instead of causing an image pull error in a roundabout way
There was a problem hiding this comment.
which pattern do you mean?
Maybe "rest of the code" would have been more appropriate, essentially the checks we perform on other parameters.
what's an example of a future change that this is protecting against?
- config is made optional/removed
- env vars / configs are empty (accidentally or by design)
that makes sense, but I think you could also just panic about it with a direct message instead of causing an image pull error in a roundabout way
We would never get a pull error since this this should never happen any way.
Maybe a better way to go about this is to just put in a test that checks for all these things, since we're already accounting for it in config logic, I don't mind either way ¯_(ツ)_/¯
Description
This change introduces the ability to pass in default images via either a configmap or env vars. Environment variables take precedence over configmaps. Environment variables follow the same nested behavior as the yaml config equivalent. For example "images.apiserver" in a
config.yamlpassed in to the operator is the equivalent to passing in an ENV VariableIMAGES_APISERVER.How Has This Been Tested?
Locally and on OCP OSD with: quay.io/hukhan/data-science-pipelines-operator:v0.0.9
Run unit tests on this pr by cloning the branch and following instructions here.
Deploy on dev cluster with ocp pipelines (logged in as cluster admin):
Once the Operator is deployed create a DSPA cr:
Deploy a sample cr with the updated resource kind
Inspect the images, now go to the DSPO deployment and change the Image ENV vars, and confirm the change is delegated to the DSPA component that was updated.
Remove the Environment variables, and confirm the configmap passed in values are consumed instead. Test this by updating the configmap after env vars are removed, and restarting the deployment.
Merge criteria: