+ init containers; use functions to build names #2
Conversation
…s in base deployment
…vice, serviceaccount, statefulsets
+ Redirect HTTP to HTTPS
tests/everything.values.yaml
Outdated
| enabled: false | ||
| loadBalancerType: ROUND_ROBIN | ||
| mtlsMode: "PERMISSIVE" | ||
| forceHttpRedirect: false |
There was a problem hiding this comment.
I think you may have added a lot of new stuff, so test case needs to be updated I think to take into account hte new stuff.
There was a problem hiding this comment.
While we're at it we might refactor tests to use https://helm.sh/docs/topics/chart_tests/
| # Configure resources it will be given with reasonable defaults | ||
| resources: | ||
| limits: | ||
| cpu: 1000m |
There was a problem hiding this comment.
I believe we established that we didn't want to set an upper cpu limit
| {{- range .Values.extraDeployment.deployments }} | ||
| {{- $deploymentName := (required "Each entry in extraDeployment.deployments needs a name" .name) }} | ||
| {{- $dictOfBasicLabels := include "chart.basicLabels" $ | fromYaml }} | ||
| {{- $dictOfBasicSelectorLabels := include "chart.basicSelectorLabels" $ | fromYaml }} |
There was a problem hiding this comment.
what are we doing to make sure cron jobs, jobs, statefulsets, and deployments don't have overlapping selector labels?
There was a problem hiding this comment.
I think just statefulsets and deployments will run into this potential issue. Jobs/cronjobs create a dynamic label if I remember correctly.
There was a problem hiding this comment.
Basic selector labels are the app and release.
Basic labels are the app, release, chart, heritage, and version.
I didn't remove anything that differentiated between apps. I only replaced groups of common labels.
You're right, though, that we should add to the statefulset and extra deployments' labels. They need a distinguishing label.
|
Picking up this PR review. I can go through and point out all the place things change for the image tag refactor, but we want to take that out. The helper function, the ability to define the tag etc. Determining the image URL is outside the scope of this chart. We intentionally built it with 1 static image URL for simplicity. In practice, the pipelines or something outside of this chart should be determining how to render the full Image url if there is business logic to it as the chart will never have all that context. So there isn't any value in all the complexity in the helper function to determine the image url with an optional tag. |
3 participants
No description provided.