Skip to content

Prevent duplicated rendering of FEATURE_ENABLE_PAYMENT #2332

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

Conversation

supersven
Copy link
Contributor

@supersven supersven commented Apr 28, 2022

If the FEATURE_ENABLE_PAYMENT feature flag is set by envVars, do not render it again. Otherwise, we might end up in confusing cases like: The flag was set to true, but it's additionally rendered with a default to false.

I gave the envVars entry priority because it's used many times in cailleach and my gut feeling tells me that it would be confusing to override an explicitly set flag with an opposite value.

Example from k8s:

kubectl get pod -n wire team-settings-598ddf7956-r6jfn -o yaml

...
spec:
  containers:
  - env:
    - name: NODE_PORT
      value: "8080"
    - name: APP_BASE
      value: https://teams.secusmart-column-1a.wire.link/
    - name: BACKEND_REST
      value: https://nginz-https.secusmart-column-1a.wire.link
    - name: BACKEND_WS
      value: wss://nginz-ssl.secusmart-column-1a.wire.link
    - name: FEATURE_ENABLE_PAYMENT
      value: "false"
    - name: FEATURE_ENABLE_PAYMENT
      value: "false"
...

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@supersven supersven temporarily deployed to cachix April 28, 2022 12:14 Inactive
@fisx
Copy link
Contributor

fisx commented Apr 29, 2022

This makes sense to me. Of course it also raises the question if we really need envVars in the first place. Do I understand correctly that this is posix/shell env, and what's under .Values.config is from values.yaml? I would strongly suggest we drop env variable handling, or at least deprecate it. But I may be missing an important use case.

@supersven
Copy link
Contributor Author

NB: I'm still testing this. Happy about your approvals. But, please don't merge, yet... ☝️

@supersven
Copy link
Contributor Author

supersven commented Apr 29, 2022

This makes sense to me. Of course it also raises the question if we really need envVars in the first place. Do I understand correctly that this is posix/shell env, and what's under .Values.config is from values.yaml? I would strongly suggest we drop env variable handling, or at least deprecate it. But I may be missing an important use case.

Hey @fisx ,

In my PIT branch of reality envVars shows up like this: https://github.com/zinfra/cailleach/blob/master/environments/secusmart-column-1a/helm_vars/wire-server/values.yaml.gotmpl#L207

It's both values.yaml stuff. I think envVars is called as it is, because it relates to the environment variables for the K8s pod of team-settings. So, it's env variables inside the deployed cluster (not on your local machine).

If the FEATURE_ENABLE_PAYMENT feature flag is set by envVars, do not
render it again. Otherwise, we might end up in confusing cases like: The
flag was set to true, but it's additionally rendered with a default to
false.
@supersven supersven force-pushed the sventennie/prevent_double_rendering_of_FEATURE_ENABLE_PAYMENT branch from ab4ffe0 to dc5eba4 Compare April 29, 2022 10:27
@supersven supersven temporarily deployed to cachix April 29, 2022 10:27 Inactive
@supersven
Copy link
Contributor Author

Tested successfully.

@supersven supersven temporarily deployed to cachix April 29, 2022 11:05 Inactive
@supersven supersven marked this pull request as ready for review April 29, 2022 11:06
@supersven supersven merged commit c183293 into develop May 2, 2022
@supersven supersven deleted the sventennie/prevent_double_rendering_of_FEATURE_ENABLE_PAYMENT branch May 2, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants