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

Configmap fix env from change #767

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Conversation

rustyShacklefurd
Copy link
Collaborator

@rustyShacklefurd rustyShacklefurd commented Dec 13, 2022

Description:
This PR modifies

  • The deployment template to use envFrom for the secret and configmap.
  • Removes annotation_JSON and will use the suggestion [here] (Helm testdrive cleanup #522 (comment))
  • Updates the configmap to quote all the values. During deployments this error [debug] ConfigMap in version "v1" cannot be handled as a ConfigMap: v1.ConfigMap.Data: ReadString: expects " or n, but found 2, error found in and kubectl apply -f templates/test.yaml The Deployment "test" is invalid: spec.template.spec.containers[0].env[15].valueFrom: Invalid value: "": may not be specified when value is not empty. Both of these error occurrence suggest that quoting values is needed for the pods to accept them correctly. Here is one of the issues being referenced.

Notes for reviewer:

WIP - rollout testing will happen 2022-12-13 and 2022-12-14
Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

…ailed rollouts, I believe we are hitting this issue - kubernetes/kubernetes#46861.  The TL;DR is that due to yaml to json conversion all ENVs need to be quoted or they cause failures to be seen.  Boolean and ints both need to be quoted along with strings; this issue was open in 2017 and still getting comments

Signed-off-by: Matt Halder <rustyShacklefurd@users.noreply.github.com>
…t that are used to set ENVs. This change should make future ENVs additions simpler

Signed-off-by: Matt Halder <rustyShacklefurd@users.noreply.github.com>
@sonarcloud
Copy link

sonarcloud bot commented Dec 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov-commenter
Copy link

Codecov Report

Base: 72.87% // Head: 72.89% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (6b8b8d0) compared to base (75dc43f).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 6b8b8d0 differs from pull request most recent head 3809ca0. Consider uploading reports for the commit 3809ca0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #767      +/-   ##
==========================================
+ Coverage   72.87%   72.89%   +0.02%     
==========================================
  Files          16       16              
  Lines        1246     1247       +1     
  Branches      231      232       +1     
==========================================
+ Hits          908      909       +1     
  Misses        280      280              
  Partials       58       58              
Impacted Files Coverage Δ
packages/relay/src/lib/eth.ts 83.75% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

DEV_MODE: {{ .Values.config.DEV_MODE | quote }}
MIRROR_NODE_RETRIES: {{ .Values.config.MIRROR_NODE_RETRIES | quote }}
MIRROR_NODE_RETRY_DELAY: {{ .Values.config.MIRROR_NODE_RETRY_DELAY | quote }}
MIRROR_NODE_LIMIT_PARAM: {{ .Values.config.MIRROR_NODE_LIMIT_PARAM | quote }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add CLIENT_TRANSPORT_SECURITY seems it was missed when added in previous PRs

Suggested change
MIRROR_NODE_LIMIT_PARAM: {{ .Values.config.MIRROR_NODE_LIMIT_PARAM | quote }}
MIRROR_NODE_LIMIT_PARAM: {{ .Values.config.MIRROR_NODE_LIMIT_PARAM | quote }}
CLIENT_TRANSPORT_SECURITY: {{ .Values.config.CLIENT_TRANSPORT_SECURITY | quote }}

@Nana-EC Nana-EC added enhancement New feature or request P1 process Build, test and deployment-process related tasks labels Dec 14, 2022
@Nana-EC Nana-EC added this to the 0.14.0 milestone Dec 14, 2022
@lukelee-sl lukelee-sl merged commit c912de4 into main Dec 19, 2022
@lukelee-sl lukelee-sl deleted the configmap-fix-envFrom-change branch December 19, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 process Build, test and deployment-process related tasks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants