Staging upgrade preparation / Sept 30th #253
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR prepares for a staging deployment upgrade scheduled for September 30th, updating the MSA platform from version 3.2.1 to 3.2.2. The upgrade includes comprehensive Docker image updates, Helm chart enhancements, and infrastructure improvements.
- Updates all MSA component Docker images to latest SHA versions for version 3.2.2
- Adds monitoring capabilities with Prometheus ServiceMonitors and postgres-exporter integration
- Implements TLS certificate support and ingress improvements for better security
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| helm/values.yaml | Core configuration updates with new image SHAs, monitoring settings, and TLS support |
| helm/Chart.yaml | Version bump from 3.2.1 to 3.2.2 |
| helm/templates/version-configmap.yaml | Updates version metadata and build number |
| helm/templates/msa-*.yaml | Adds monitoring endpoints, TLS configuration, and deployment improvements |
| docker-compose*.yml | Updates all Docker images to new SHA versions for 3.2.2 release |
| helm/images.application.txt | Updates image references to match new versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| tls: | ||
| {{- toYaml .Values.global.tls | nindent 4 }} | ||
| {{- end }} | ||
| spec: |
There was a problem hiding this comment.
Duplicate 'spec:' declaration. Line 65 already declares spec, making line 70 invalid YAML.
| spec: |
| image: ubiqube/msa2-monitoring:sha-3ed8905abee88ca8f9058f341913f704d2db12fc | ||
| parameters: | ||
| amqp_address: syslogs | ||
| amqp_address: "core-engine.monitoring" |
There was a problem hiding this comment.
The amqp_address value changed from 'syslogs' to 'core-engine.monitoring'. Ensure this matches the broker queue configuration and that dependent services are updated accordingly.
| amqp_address: "core-engine.monitoring" | |
| amqp_address: "syslogs" |
vmonnier
left a comment
vmonnier
left a comment
There was a problem hiding this comment.
Docker images should be publically available on openmsa/openmsa repo, not on ubiqube one
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| tls: | ||
| {{- toYaml .Values.global.tls | nindent 4 }} | ||
| {{- end }} | ||
| spec: |
There was a problem hiding this comment.
Duplicate 'spec:' declaration on lines 65 and 70. The second 'spec:' declaration will override the first one, potentially causing the TLS configuration to be ignored.
| spec: |
| {{- if .Values.msa_zipkin.enable }} | ||
| - name: _JAVA_OPTIONS | ||
| value: "-XX:MaxRAM=2g" | ||
| {{- if .Values.msa_zipkin.enabled }} |
There was a problem hiding this comment.
Inconsistent property name used. Line 28 uses '.Values.msa_zipkin.enabled' but line 41 in msa-api.yaml uses '.Values.msa_zipkin.enable' (without 'd'). This inconsistency could cause the zipkin configuration to not work properly in one of the services.
| {{- if .Values.msa_zipkin.enabled }} | |
| {{- if .Values.msa_zipkin.enable }} |
| {{- if .Values.global.tls }} | ||
| host: {{ index (index .Values.global.tls 0).hosts 0 | quote }} | ||
| {{- end }} | ||
| --- |
There was a problem hiding this comment.
Extra '---' separator on line 224 that serves no purpose. This should be removed to clean up the YAML structure.
| {{- if .Values.msa_rsyslog.metallb}} | ||
| loadBalancerIP: {{ index .Values.msa_front.externalIPs 0 }} | ||
| {{- end }} | ||
| {{- if and (not .Values.msa_rsyslog.metallb) .Values.msa_front.externalIPs }} |
There was a problem hiding this comment.
Inconsistent property reference. The condition checks for '.Values.msa_front.externalIPs' but the context suggests it should be '.Values.msa_rsyslog.externalIPs' since this is configuring the rsyslog service, not the front service.
| {{- if and (not .Values.msa_rsyslog.metallb) .Values.msa_front.externalIPs }} | |
| {{- if and (not .Values.msa_rsyslog.metallb) .Values.msa_rsyslog.externalIPs }} |
| {{- if ( .Values.global.artemis.port.core ) -}} | ||
| {{ (print ":" .Values.global.artemis.port.core) }} | ||
| {{- end -}} | ||
| {{ (print "?consumerWindowSize=0") -}} |
There was a problem hiding this comment.
[nitpick] The hard-coded query parameter should be made configurable through values.yaml to allow different environments to use different consumer window sizes if needed.
| {{ (print "?consumerWindowSize=0") -}} | |
| {{- $consumerWindowSize := default 0 .Values.global.artemis.consumerWindowSize -}} | |
| {{ (print "?consumerWindowSize=" $consumerWindowSize) -}} |
No description provided.