Update mc-router chart for new auto scale options #270
Conversation
|
@itzg I verified |
|
@itzg This is ready for review! Absolutely no worries if you don't have time, I'm just excited to start using the new version once it's merged 😄 |
| @@ -8,7 +8,7 @@ rules: | |||
| - apiGroups: [""] | |||
| resources: ["services"] | |||
| verbs: ["watch","list"] | |||
| {{- if .Values.minecraftRouter.autoScaleUp.enabled }} | |||
| {{- if .Values.minecraftRouter.autoScale.up.enabled }} | |||
There was a problem hiding this comment.
Unfortunately this would be a breaking change for existing chart users. You could do this as an or-expression to allow for both old and new value identifiers.
| annotations: | ||
| {{- if .Values.minecraftRouter.autoScale.allowDeny }} | ||
| checksum/secret-config: {{ include (print .Template.BasePath "/secret.yaml") . | sha256sum }} |
There was a problem hiding this comment.
I'm not sure from just this how the annotation name and secrets file relates to allow/deny list.
There was a problem hiding this comment.
This is done because for now, this secret only exists if the user specifies an auto scale allow/deny list. If the secret does exist, and the users makes changes to that list then the mc-router pod needs to be restarted to receive the changes which is what the annotation triggers. I could instead make this a TPL function called secretEnabled or something that simply wraps .Values.minecraftRouter.autoScale.allowDeny to better clarify intent?
There was a problem hiding this comment.
Aha, I had missed the sha operation, so now I understand and agree with the "checksum/" prefix. Otherwise perhaps a more descriptive name like "checksum/allow-deny-secret" would make me all the way happy.
| @@ -54,7 +59,12 @@ spec: | |||
| {{- include "mc-router.envMap" (list "PORT" $minecraftPort) }} | |||
|
|
|||
| {{- with .Values.minecraftRouter }} | |||
| {{- include "mc-router.envMap" (list "AUTO_SCALE_UP" .autoScaleUp.enabled) }} | |||
| {{- include "mc-router.envMap" (list "AUTO_SCALE_UP" .autoScale.up.enabled) }} | |||
| volumeMounts: | ||
| {{- if .Values.minecraftRouter.autoScale.allowDeny }} | ||
| - name: secret-config |
| volumes: | ||
| {{- if .Values.minecraftRouter.autoScale.allowDeny }} | ||
| - name: secret-config |
There was a problem hiding this comment.
Maybe this helps answer my previous comments, however I'm not sure if that file needs to be a secret and I'm not sure if this is forcing too much convention on the users.
There was a problem hiding this comment.
I think it could either be a secret or config map. If you would prefer, I'm happy to make that an additional option in the values.
There was a problem hiding this comment.
Yeah, if it doesn't cause too much of a mess with the values and template, it would be nice to give the option of secret or configmap. Otherwise, just as a secret now seems decent to me.
| @@ -60,16 +60,44 @@ | |||
| "minecraftRouter": { | |||
| "type": "object", | |||
| "properties": { | |||
| "autoScaleUp": { | |||
There was a problem hiding this comment.
Regarding my backward compatibility commments, I'm not sure if the old value/object needs to be also included here. I'd have to look if JSON schema has a deprecated flag.
There was a problem hiding this comment.
(This applies to all other backwards compatible comments). I'm happy to make changes to allow for backwards compatibility, but wondering how you would feel about simply bumping the chart major version and leaving this as a breaking change? I completely understand if you would prefer to make it backwards compatible, just wanted to check first 😄
There was a problem hiding this comment.
Good point, that is supposed to be point of major versions 😀 Even a minor version bump is probably somewhat legit for just an interface change.
How about this:
- keep it as a minor version bump
- don't worry about backward compatible handling
- could place somewhere a condition and
failif the old value is set. (I'm not sure the best convention of where to declare such template guards)
There was a problem hiding this comment.
I think that sounds good, I'll take a look at the JSON Schema because I think the additionalProperties key being false should guard against that.
|
Renamed the secret/configmap objects to better clarify their purpose. Users can now choose to use a Secret or ConfigMap for storing allow/deny list config. Also, with the addition of the |
Fixes #269