argo-rollouts icon indicating copy to clipboard operation
argo-rollouts copied to clipboard

feat: Configure metric provider connection information via env vars

Open RaviHari opened this issue 2 years ago • 4 comments

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.

RaviHari avatar Jun 07 '22 17:06 RaviHari

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 2 Code Smells

No Coverage information No Coverage information
4.1% 4.1% Duplication

sonarqubecloud[bot] avatar Jun 07 '22 17:06 sonarqubecloud[bot]

Codecov Report

Merging #2080 (08e2449) into master (d93b043) will decrease coverage by 0.04%. The diff coverage is 85.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.

codecov[bot] avatar Jun 07 '22 18:06 codecov[bot]

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: https://github.com/argoproj/argo-rollouts/pull/1956#issuecomment-1098460062

RaviHari avatar Jun 08 '22 03:06 RaviHari

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.

zachaller avatar Jun 08 '22 13:06 zachaller