-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AnalysisTemplate & update logic for canary rollout strategy #46
base: master
Are you sure you want to change the base?
Conversation
13dc14d
to
cf39cf4
Compare
175a67c
to
9f69f14
Compare
{{- with .Values.rollout.notifications }} | ||
{{- range $event, $channels := . }} | ||
{{- range $channel := $channels }} | ||
notifications.argoproj.io/subscribe.{{ $event }}.slack: {{ join ";" $channels | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to keep this more generalized, since not everybody uses Slack for notifications. Perhaps we could just use the existing annotations
for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I initially wanted to do but it would add the annotation to all resources while it would be only needed for Rollout
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generic-service applies labels
to all resources but annotations
only to the resource created in controller.yaml
, Rollout
in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it 👍
b71ef98
to
48efa9e
Compare
{{- with .Values.rollout.analysis -}} | ||
analysis: {{- . | toYaml | nindent 8 }} | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with
doesn't seem necessary here.
Indent seems to be too deep. Would put analysis
under trafficRouting
.
{{- with .Values.rollout.analysis -}} | |
analysis: {{- . | toYaml | nindent 8 }} | |
{{- end }} | |
analysis: {{- .Values.rollout.analysis | toYaml | nindent 8 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
analysis
is an optional field (with or without traffic routing). see rollout specs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then perhaps like this?
{{- with .Values.rollout.analysis -}} | |
analysis: {{- . | toYaml | nindent 8 }} | |
{{- end }} | |
{{- if .Values.rollout.analysis }} | |
analysis: {{- . | toYaml | nindent 8 }} | |
{{- end }} |
17189bc
to
9c99703
Compare
4a8f6da
to
4800307
Compare
Changes made:
stable(Service|Metadata)
andcanary(Service|Metadata)
depending on whetheruseTrafficRouting
istrue
for a canary rolloutuseTrafficRouting
istrue
orrollout.strategy
isBlueGreen
steps
to control progress of canary rolloutsAnalysisTemplate
progressDeadlineSeconds
with default kubernetes valueminReadySeconds
with default kubernetes valueprogressDeadlineAbort
for argo rollouts