add annotation namespace #31
Conversation
|
Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄 |
like-inspur
changed the title
There was a problem hiding this comment.
lgtm, comments inline! Thanks for the PR!
Of note: we are currently in a transition period away from Yahoo's CLA to a new CLA for Automattic. We should have the CLA sorted out in a week or so, so this PR will have to wait to merge until we get that sorted out.
examples/kubernetes/deployment.yaml
Outdated
| scheme: HTTPS | ||
| path: /health | ||
| port: https | ||
| initialDelaySeconds: 10 |
There was a problem hiding this comment.
I do not think theres any reason to delay readiness checks. What do you think?
examples/kubernetes/deployment.yaml
Outdated
| @@ -64,3 +72,5 @@ spec: | |||
| value: "conf/" | |||
| - name: "CONFIGMAP_LABELS" | |||
| value: "app=k8s-sidecar-injector" | |||
| - name: "CONFIGMAP_NAMESPACE" | |||
There was a problem hiding this comment.
This will work.
I was thinking (in order to provide a smoother out of the box experience) that we could change https://github.com/tumblr/k8s-sidecar-injector/blob/master/entrypoint.sh#L21 to omit the --configmap-namespace flag if CONFIGMAP_NAMESPACE is empty. This will use the namespace the injector is deployed into automatically, removing some config duplication and Doing The Right Thing ™️ in common cases.
|
@like-inspur thanks for citing this from the other issue. I dropped comments for things we would like changed before merging this. Would you be able to address them? Alternatively, if you would like to break your change in 7105418 out into a separate PR, we can merge that easily and keep it uncoupled from the rest of this PR? Thanks in advance! |
like-inspur
changed the title
|
@byxorna I have remove other change and only save this, you can merge directly |
|
@like-inspur thanks for rebasing this PR, and thank you for the contribution! Your changes will be available in the |
1、current deployment only has livenessProbe，but don't have readiness
2、configmap read config form installation config, so it's better to config this variable
3、execurte need ruby env, so tell reader to install ruby first