argo-rollouts
argo-rollouts copied to clipboard
feat: Configure metric provider connection information via env vars
fixes: https://github.com/argoproj/argo-rollouts/issues/1979
Checklist:
- [X] Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
- [X] The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g.
"fix(controller): Updates such and such. Fixes #1234"
. - [X] I've signed my commits with DCO
- [X] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
- [X] My builds are green. Try syncing with master if they are not.
- [X] My organization is added to USERS.md.
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells
No Coverage information
4.1% Duplication
Codecov Report
Merging #2080 (08e2449) into master (d93b043) will decrease coverage by
0.04%
. The diff coverage is85.88%
.
@@ Coverage Diff @@
## master #2080 +/- ##
==========================================
- Coverage 82.42% 82.37% -0.05%
==========================================
Files 120 120
Lines 17431 17444 +13
==========================================
+ Hits 14367 14369 +2
- Misses 2352 2362 +10
- Partials 712 713 +1
Impacted Files | Coverage Δ | |
---|---|---|
utils/rollout/rolloututil.go | 86.71% <0.00%> (-5.02%) |
:arrow_down: |
metricproviders/wavefront/wavefront.go | 89.70% <80.00%> (-4.70%) |
:arrow_down: |
metricproviders/datadog/datadog.go | 77.34% <100.00%> (-0.59%) |
:arrow_down: |
metricproviders/newrelic/newrelic.go | 92.62% <100.00%> (+0.65%) |
:arrow_up: |
metricproviders/prometheus/prometheus.go | 96.19% <100.00%> (-0.24%) |
:arrow_down: |
utils/record/record.go | 81.33% <0.00%> (+0.77%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update d93b043...08e2449. Read the comment docs.
Instead of informing people in the docs to add individual env vars to the deployment. I wonder if it make more sense to setup the deployment with say a metrics-config-env configmap then using
envFrom:
to include it into the deployment manifests https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variablesI would also probably use a k8s Secret instead of a configmap or at the least setup both.
Here is the discussion on why this approach is chosen: https://github.com/argoproj/argo-rollouts/pull/1956#issuecomment-1098460062
Instead of informing people in the docs to add individual env vars to the deployment. I wonder if it make more sense to setup the deployment with say a metrics-config-env configmap then using
envFrom:
to include it into the deployment manifests https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variables I would also probably use a k8s Secret instead of a configmap or at the least setup both.Here is the discussion on why this approach is chosen: #1956 (comment)
Yup I am totally for that method I just think the way it is documented here is not ideal. Let me try to be more direct with the info from the k8s doc. Instead of having to add a whole bunch of env vars to the deployment manifest for each configuration what I am suggesting is using k8s envFrom:
as a quick example from the docs page.
apiVersion: v1
kind: Secret
metadata:
name: datadog-secret
type: Opaque
data:
ARGO_ROLLOUTS_DD_ADDRESS: https://api.datadoghq.com
ARGO_ROLLOUTS_DD_API_KEY: <datadog-api-key>
ARGO_ROLLOUTS_DD_APP_KEY: <datadog-app-key>
---
spec:
containers:
- name: test-container
image: k8s.gcr.io/busybox
command: [ "/bin/sh", "-c", "env" ]
envFrom:
- configMapRef:
name: datadog-secret
- secretRef:
name: datadog-secret
That is one option that is very specific still you could take that one step farther and make it very generic and we could include both an empty secret and configmap and include it in the manifests and then setup the deployment to mount both of them then end users would just need to patch or add their own config to the config map and not touch the deployment at all.