Skip to content
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 cluster name to flagger cmd args for altering #1041

Merged
merged 2 commits into from
Jan 11, 2022

Conversation

baldey-nz
Copy link
Contributor

@baldey-nz baldey-nz commented Oct 26, 2021

Signed-off-by: baldey-nz baldey@gmail.com

As per issue feature request #926, the notifications to slack for flagger are great but if we have multiple clusters running flagger and all are sending notifications to the same channel then its hard to determine which cluster the canary is happening in from the notification msgs.
This change adds an optional summary field (Spec.Summary)
If this fields isnt set then nothing changes in the notifications.

If summary is set then a new summary field is created e.g:

my-app.my-namespace
New Deployment detected, initialization completed.
Summary
Sandbox env

Target
Deployment/my-app.my-namespace
.......

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the cluster field and leave only summary. In a distant feature, Flagger will be integrated with Flux notification-controller and the Alert CRD from Flux contains only Summary.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2021

Codecov Report

Merging #1041 (8c881ab) into main (01d4780) will increase coverage by 0.28%.
The diff coverage is 12.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1041      +/-   ##
==========================================
+ Coverage   56.88%   57.17%   +0.28%     
==========================================
  Files          76       76              
  Lines        6093     6169      +76     
==========================================
+ Hits         3466     3527      +61     
- Misses       2102     2111       +9     
- Partials      525      531       +6     
Impacted Files Coverage Δ
pkg/controller/controller.go 0.72% <0.00%> (-0.01%) ⬇️
pkg/controller/events.go 52.89% <14.28%> (-2.76%) ⬇️
pkg/router/kubernetes_default.go 63.43% <0.00%> (-0.69%) ⬇️
pkg/canary/config_tracker.go 82.00% <0.00%> (-0.51%) ⬇️
pkg/router/istio.go 81.07% <0.00%> (-0.48%) ⬇️
pkg/router/util.go 100.00% <0.00%> (ø)
pkg/router/appmesh_v1beta2.go 88.12% <0.00%> (+0.30%) ⬆️
pkg/router/contour.go 88.54% <0.00%> (+0.36%) ⬆️
pkg/router/appmesh.go 85.42% <0.00%> (+0.45%) ⬆️
pkg/canary/daemonset_controller.go 58.33% <0.00%> (+0.64%) ⬆️
... and 3 more

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 01d4780...8c881ab. Read the comment docs.

…summary for notification purposes

Signed-off-by: baldey-nz <baldey@gmail.com>
@baldey-nz baldey-nz force-pushed the baldey-nz/notification-change branch from 7189eb5 to c638edd Compare October 27, 2021 18:14
@baldey-nz
Copy link
Contributor Author

baldey-nz commented Oct 27, 2021

Cluster field removed as requested. @stefanprodan any chance of taking another look at this one?

@baldey-nz baldey-nz changed the title If applied, this commit will add optional canary spec fields, summary… add optional canary spec field summary… Nov 3, 2021
@Nerja
Copy link
Contributor

Nerja commented Nov 21, 2021

@stefanprodan Should we maybe place the summary field in the CanaryAlert struct(

type CanaryAlert struct {
) instead of only providing the ability to specify a summary field per canary? Enabling the users to define a summary per alert is more in line with what is supported by the Flux Alert API.

@baldey-nz
Copy link
Contributor Author

Hi @stefanprodan - any chance you could let us know what the preferred option is here please? we are really keen to start using flagger in production but this is currently blocking us. thanks!

@stefanprodan
Copy link
Member

@Nerja good point, adding it to Canary means we'll break the API once we switch over to Flux notification-controller.

@baldey-nz to unblock this, I'm for adding a --cluster-name flag to Flagger, if that flag has a value, then we inject it as a cluster filed in the alert.

@baldey-nz
Copy link
Contributor Author

@stefanprodan like this?

Signed-off-by: baldey-nz <baldey@gmail.com>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @baldey-nz

@stefanprodan stefanprodan changed the title add optional canary spec field summary… Add cluster name flag to flagger cmd args for altering Jan 6, 2022
@stefanprodan stefanprodan added the kind/enhancement Improvement request for an existing feature label Jan 6, 2022
@stefanprodan stefanprodan changed the title Add cluster name flag to flagger cmd args for altering Add cluster name to flagger cmd args for altering Jan 6, 2022
@stefanprodan stefanprodan merged commit 5776f0b into fluxcd:main Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvement request for an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants