Add extraEnvConfigMaps and document some values
#3691
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
|
@gjtempleton I'm unsure what version increase to use, since this "only" is a breaking change for people who use the undocumented |
There was a problem hiding this comment.
Hi, thanks for the PR!
Good point about the versioning and breaking of the existing functionality being undocumented.
I've already raised #3679 bumping the major version from 1.X to 9.X and I think this change is best included at the same time as also potentially breaking functionality. If you could add an entry to the Readme via the .gotmpl file to highlight the change happening and how users can maintain their existing behaviour I think that's the best approach. I've also got one quick nitpick.
| - secretRef: | ||
| name: {{ .Values.envFromSecret }} | ||
| {{- else if .Values.envFromConfigMap }} |
There was a problem hiding this comment.
Do we want this to be an else if? Could a user not conceivably want to define both envFromSecret and envFromConfigMap?
There was a problem hiding this comment.
Allowing both is better, you're right. I'll change it.
avorima
force-pushed
the
document-extra-envs
branch
from
bc41d42 to
e46e3e2
Compare
k8s-ci-robot
added
size/M
avorima
force-pushed
the
document-extra-envs
branch
from
e46e3e2 to
0fb17f4
Compare
k8s-ci-robot
added
the
needs-rebase
avorima
force-pushed
the
document-extra-envs
branch
from
0fb17f4 to
d183b2a
Compare
k8s-ci-robot
removed
the
needs-rebase
There already is `envFromSecret` and `extraEnvSecrets`, so I made `envFromConfigMap` behave like `envFromSecret` and added `extraEnvConfigMaps` to replace it. This pattern should make the intended use of the values clearer.
avorima
force-pushed
the
document-extra-envs
branch
from
d183b2a to
0364768
Compare
|
@gjtempleton I've added a short migration guide, but I still felt like this change should at least be a minor version increase. WDYT? |
|
@avorima Agreed on the minor release, thanks for the rebase. Sorry, I should have got this merged last week before the 9.0.0 release. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avorima, gjtempleton 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 |
k8s-ci-robot
added
the
approved
There already is
envFromSecretandextraEnvSecrets, so I madeenvFromConfigMapbehave likeenvFromSecretand addedextraEnvConfigMapsto replace it.This pattern should make the intended use of the values clearer.