Better slack notifications #415

paulbellamy · 2017-02-01T18:06:01Z

Fixes: #390

TODO:

Clean up TODOs in the code
Some sort of migration from jobs? or fill the release struct to send to notifiers
Make sure the serviceSpec stuff migration works, or move it to separate pr.
Figure out how to integrate it with michael's changes for job results...

paulbellamy · 2017-02-03T16:16:39Z

serviceSpec stuff moved to: #423

paulbellamy · 2017-02-06T14:03:38Z

Note, without more info from the release, this doesn't currently include any results in the output.

* Rename ReleaseParams -> ReleaseSpec * Add tests for slack notifier

squaremo

There's a test failing, but it looks easily fixed.
Nitpicks only, really -- though TODOs remain.

release.go

+	"github.com/weaveworks/flux/guid"
+)
+
+const (


release.go

+	EndedAt   time.Time            `json:"endedAt"`
+	Done      bool                 `json:"done"`
+	Priority  int                  `json:"priority"`
+	Status    ServiceReleaseStatus `json:"status"`


jobs/job.go

-	// TODO: Remove this once there are no more jobs with ServiceSpec, only ServiceSpecs
-	ServiceSpec flux.ServiceSpec
-}
+type ReleaseJobParams flux.ReleaseSpec


* uses spec instead of actual results. * don't notify slack for dry-runs

paulbellamy · 2017-02-07T14:03:21Z

I re-did the default template, to use the spec instead of the results to output stuff.

It means the output is based on what was requested, not what was done, but... that is what we have for now (until the result format changes from @squaremo's work).

for example:

Release no updates to flux-example/nginx. done
Release all latest to flux-example/nginx. done
Release quay.io/weaveworks/helloworld:master-a000001 to all. done
Release all latest to flux-example/nginx. some error here. failed
Release all latest to all. done // This one is a bit unsatisfying...

paulbellamy · 2017-02-07T14:06:14Z

Also, I think all the TODOs left (now) are places where we need to integrate this with your changes, @squaremo, or reminders to remove deprecated stuff.

release/releaser.go

+			EndedAt:  time.Now().UTC(),
+			Done:     true,
+			Priority: job.Priority,
+			Status:   flux.ServiceReleaseStatus(job.Status),


squaremo · 2017-02-07T16:03:20Z

I re-did the default template, to use the spec instead of the results to output stuff.

Does this more or less replicate the current behaviour?

squaremo

Assuming it makes the notifications act mostly as they are now, until there are results to put in them: looks good! ⭐

paulbellamy force-pushed the 390-better-notifications branch from 17e724d to 43d7fdc Compare February 3, 2017 15:02

paulbellamy changed the title ~~WIP -- Better slack notifications~~ Better slack notifications Feb 3, 2017

paulbellamy force-pushed the 390-better-notifications branch from 9b569f6 to 474d379 Compare February 3, 2017 16:26

squaremo self-assigned this Feb 3, 2017

paulbellamy force-pushed the 390-better-notifications branch from 8c23bcb to 7fcfacd Compare February 6, 2017 13:57

Better release slack notifications, once per release

930c7b0

* Rename ReleaseParams -> ReleaseSpec * Add tests for slack notifier

paulbellamy force-pushed the 390-better-notifications branch from 5785a16 to 930c7b0 Compare February 6, 2017 14:08

squaremo reviewed Feb 6, 2017

View reviewed changes

squaremo removed their assignment Feb 6, 2017

a notification format which works for currently available data

e430b63

* uses spec instead of actual results. * don't notify slack for dry-runs

paulbellamy force-pushed the 390-better-notifications branch from 87b8ce7 to e430b63 Compare February 7, 2017 14:02

Review Feedback - naming consistency

d7098e6

squaremo reviewed Feb 7, 2017

View reviewed changes

release/releaser.go Outdated

EndedAt: time.Now().UTC(),

Done: true,

Priority: job.Priority,

Status: flux.ServiceReleaseStatus(job.Status),

This comment was marked as abuse.

Sign in to view

squaremo approved these changes Feb 7, 2017

View reviewed changes

Use the flux.ReleaseStatus enum to avoid a slip-up

d13520d

paulbellamy force-pushed the 390-better-notifications branch from 9d7dae2 to d13520d Compare February 8, 2017 10:03

paulbellamy merged commit 1ec8e4b into master Feb 8, 2017

paulbellamy deleted the 390-better-notifications branch February 8, 2017 10:17

paulbellamy mentioned this pull request Feb 8, 2017

Async releases and shutdown #432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better slack notifications #415

Better slack notifications #415

paulbellamy commented Feb 1, 2017 •

edited

paulbellamy commented Feb 3, 2017

paulbellamy commented Feb 6, 2017

squaremo left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

paulbellamy commented Feb 7, 2017

paulbellamy commented Feb 7, 2017

This comment was marked as abuse.

squaremo commented Feb 7, 2017

squaremo left a comment •

edited

Better slack notifications #415

Better slack notifications #415

Conversation

paulbellamy commented Feb 1, 2017 • edited

TODO:

paulbellamy commented Feb 3, 2017

paulbellamy commented Feb 6, 2017

squaremo left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

paulbellamy commented Feb 7, 2017

paulbellamy commented Feb 7, 2017

This comment was marked as abuse.

squaremo commented Feb 7, 2017

squaremo left a comment • edited

Choose a reason for hiding this comment

paulbellamy commented Feb 1, 2017 •

edited

squaremo left a comment •

edited