Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Rename the annotation used to inject the proxy #49

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vbehar
Copy link
Contributor

@vbehar vbehar commented Oct 24, 2019

Following #47 here, let's rename the annotation used to inject the proxy in the pods:

  • previous annotation key: osiris.deislabs.io/enabled
  • new annotation key: osiris.deislabs.io/injectProxy

so that we can avoid confusion between the deployments and pods annotations

Following deislabs#47 here, let's rename the annotation used to inject the proxy in the pods:

- previous annotation key: `osiris.deislabs.io/enabled`
- new annotation key: `osiris.deislabs.io/injectProxy`

so that we can avoid confusion between the deployments and pods annotations
@krancour
Copy link
Contributor

@vbehar I'm wondering about maybe using an annotation that references purpose more than implementation. What about osiris.deislabs.io/collectMetrics? That doesn't say how the metrics are collected and doesn't invite users to concern themselves with that detail.

@codecov-io
Copy link

Codecov Report

Merging #49 into master will decrease coverage by 0.05%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
- Coverage   58.77%   58.72%   -0.06%     
==========================================
  Files          11       11              
  Lines         638      642       +4     
==========================================
+ Hits          375      377       +2     
- Misses        234      236       +2     
  Partials       29       29
Impacted Files Coverage Δ
pkg/kubernetes/osiris.go 94.44% <60%> (-5.56%) ⬇️

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 3f71d07...ef8e139. Read the comment docs.

@vbehar
Copy link
Contributor Author

vbehar commented Oct 25, 2019

@krancour yes, good idea

@krancour
Copy link
Contributor

@vbehar thanks for updating this to osiris.deislabs.io/collectMetrics. I think the original, ambiguous osiris.deislabs.io/enabled still appears on deployment and service resources. I'm thinking they deserve the same treatment as part of this PR. Something like osiris.deislabs.io/manageEndpoints for service resources and something like osiris.deislabs.io/enableScaling for deployments.

vbehar added a commit to dailymotion-oss/osiris that referenced this pull request Sep 14, 2020
vbehar added a commit to dailymotion-oss/osiris that referenced this pull request Sep 15, 2020
see deislabs#49 (comment)

no more `osiris.xxx/enabled` annotation, it's been replaced by:
- `osiris.xxx/collectMetrics` for the pods
- `osiris.xxx/enableScaling` for the workloads (deployments/statefulsets)
- `osiris.xxx/manageEndpoints` for the services
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants