Added ignore event types into notifications #2501

xiaonancc77 · 2018-01-18T10:39:41Z

Because image pull events occur frequently and not very important, we do not want to receive such notification events. So we need a filter that filters out the types of events I do not need, and the user can customize the types of events I do not want.

GordonTheTurtle · 2018-01-18T10:39:43Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "master" [email protected]:xiaonancc77/distribution.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

codecov · 2018-01-18T10:53:25Z

Codecov Report

Merging #2501 into master will decrease coverage by 9.63%.
The diff coverage is 95%.

@@            Coverage Diff            @@
##           master   #2501      +/-   ##
=========================================
- Coverage   60.83%   51.2%   -9.64%     
=========================================
  Files         129     129              
  Lines       11864   11830      -34     
=========================================
- Hits         7218    6057    -1161     
- Misses       3743    5012    +1269     
+ Partials      903     761     -142

Impacted Files	Coverage Δ
configuration/configuration.go	`68.33% <ø> (ø)`	⬆️
registry/handlers/app.go	`48.04% <0%> (+0.24%)`	⬆️
notifications/sinks.go	`95% <100%> (+0.33%)`	⬆️
notifications/endpoint.go	`94.44% <100%> (+0.15%)`	⬆️
registry/storage/driver/gcs/gcs.go	`0.39% <0%> (-68.67%)`	⬇️
registry/storage/driver/oss/oss.go	`0.56% <0%> (-56.91%)`	⬇️
registry/storage/driver/s3-goamz/s3.go	`0.5% <0%> (-51.14%)`	⬇️
registry/storage/driver/s3-aws/s3.go	`4.15% <0%> (-50.98%)`	⬇️
registry/client/transport/transport.go	`69.69% <0%> (-9.1%)`	⬇️
registry/storage/driver/base/base.go	`69.23% <0%> (-5.32%)`	⬇️
... and 6 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 6fca8d6...fc1d364. Read the comment docs.

stevvooe · 2018-01-18T18:33:35Z

configuration/configuration.go

@@ -551,6 +551,7 @@ type Endpoint struct {
 	Timeout           time.Duration `yaml:"timeout"`           // HTTP timeout
 	Threshold         int           `yaml:"threshold"`         // circuit breaker threshold before backing off on failure
 	Backoff           time.Duration `yaml:"backoff"`           // backoff duration
+	IgnoreEventTypes  []string      `yaml:"ignoreEventTypes"`  // ignore event types


We don't use camelcase here.

OK, I'll modify it.

stevvooe · 2018-01-18T18:36:21Z

Rather than piling on another field, it would better if this was properly structured:

ignore:
  mediatypes: []
  actions: []

It needs to be made clear that if it matches any of the mediatypes or actions, that it won't be propagated.

This would deprecate ignoredmediatypes in favor of this configuration, but this is going to get out of hand really quickly. #1957 should have considered these cases before being merged.

Other than that, this seems like a reasonable feature.

xiaonancc77 · 2018-01-22T02:37:28Z

Thancks@stevvooe , this is a good idea, I will update the commit and submit it later.

xiaonancc77 · 2018-01-24T09:44:45Z

@stevvooe I don't how to solve this problem.

The diff hit index is lower, because I formatted my code with "gofmt -s" to pass the CI test. I do not know how to improve the diff hit index for my code. Hope to get your help, Thanks.

xiaonancc77 · 2018-01-26T09:39:59Z

@stevvooe Hope to get your help, Thanks so much.

stevvooe · 2018-02-03T00:00:09Z

@xiaonancc77 Don't worry about the diffs: we deal with this in Go to have consistent formatting.

stevvooe · 2018-02-03T00:00:41Z

configuration/configuration.go

-	Timeout           time.Duration `yaml:"timeout"`           // HTTP timeout
-	Threshold         int           `yaml:"threshold"`         // circuit breaker threshold before backing off on failure
-	Backoff           time.Duration `yaml:"backoff"`           // backoff duration
-	IgnoredMediaTypes []string      `yaml:"ignoredmediatypes"` // target media types to ignore


You need to preserve this field to maintain backwards compatibility. Just enforce that one or the other needs to be used and not both.

stevvooe · 2018-02-03T00:00:49Z

configuration/configuration.go

+}
+
+//Ignore configures mediaTypes and actions of the event, that it won't be propagated
+type Ignore struct {


stevvooe · 2018-02-03T00:02:08Z

notifications/endpoint.go

 	"net/http"
 	"time"
 )

 // EndpointConfig covers the optional configuration parameters for an active
 // endpoint.
 type EndpointConfig struct {
+	configuration.Ignore


Field embedding isn't quite need here. Should be Ignore configuration.Ignore.

stevvooe · 2018-02-03T00:04:37Z

notifications/http.go

 	// TODO(stevvooe): Allow one to configure the media type accepted by this
 	// sink and choose the serialization based on that.
 }

 // newHTTPSink returns an unreliable, single-flight http sink. Wrap in other
 // sinks for increased reliability.
-func newHTTPSink(u string, timeout time.Duration, headers http.Header, transport *http.Transport, listeners ...httpStatusListener) *httpSink {
+func newHTTPSink(u string, timeout time.Duration, headers http.Header, transport *http.Transport, iActions []string, listeners ...httpStatusListener) *httpSink {


Sinks are compositional and should be focused on a single goal. Rather than adding an argument here, fix newIgnoredMediaTypesSink to handle this.

Thanks，I will update the commit and submit it later. O(∩_∩)O

stevvooe · 2018-02-03T00:04:51Z

@nwt PTAL

nwt · 2018-02-06T23:58:52Z

Same comments as @stevvooe, so LGTM after those are addressed.

xiaonancc77 · 2018-02-27T14:09:22Z

@stevvooe @nwt I'm so sorry to submit late. Because I am very busy recently. I have already solved the above problems. Thanks. O(∩_∩)O

stevvooe · 2018-03-09T23:38:34Z

LGTM

nwt

LGTM.

nwt · 2018-03-09T23:50:35Z

notifications/sinks.go

@@ -210,14 +210,15 @@ func (eq *eventQueue) next() []Event {
 	return block
 }

-// ignoredMediaTypesSink discards events with ignored target media types and
+// ignoredSink discards events with ignored target media types and anctions.


Nit: s/anctions/actions/.

nwt · 2018-03-09T23:59:01Z

notifications/endpoint.go

+		endpoint.Sink = newIgnoredSink(endpoint.Sink, config.Ignore.MediaTypes, config.Ignore.Actions)
+	} else {
+		endpoint.Sink = newIgnoredSink(endpoint.Sink, config.IgnoredMediaTypes, config.Ignore.Actions)
+	}


One other nit: If both config.Ignore.MediaTypes and config.IgnoredMediaTypes are set, I'd expect both to be honored, so I'd do something like this here:

mediaTypes := append(config.Ignore.MediaTypes, config.IgnoredMediaTypes...) endpoint.Sink = newIgnoredSink(endpoint.Sink, mediaTypes, config.Ignore.Actions)

It's a good idea.

Signed-off-by: elsanli(李楠) <[email protected]>

xiaonancc77 · 2018-03-13T08:52:27Z

@nwt I have already updated my code. Please review it~

nwt · 2018-03-13T08:55:18Z

LGTM.

dmcgowan · 2018-03-14T17:36:57Z

LGTM

GordonTheTurtle added the dco/no label Jan 18, 2018

xiaonancc77 force-pushed the master branch from acae93c to e99d5d4 Compare January 18, 2018 10:53

GordonTheTurtle removed the dco/no label Jan 18, 2018

xiaonancc77 force-pushed the master branch from e99d5d4 to 309585c Compare January 18, 2018 11:10

stevvooe reviewed Jan 18, 2018

View reviewed changes

xiaonancc77 force-pushed the master branch 2 times, most recently from 41cfa83 to 78fef0a Compare January 22, 2018 07:21

GordonTheTurtle added the dco/no label Jan 22, 2018

xiaonancc77 force-pushed the master branch from 1c34712 to 1205df7 Compare January 22, 2018 08:56

GordonTheTurtle added the dco/no label Jan 22, 2018

xiaonancc77 force-pushed the master branch from fca32d5 to c9902e0 Compare January 22, 2018 10:04

GordonTheTurtle added the dco/no label Jan 22, 2018

xiaonancc77 force-pushed the master branch from c9902e0 to dd6becc Compare January 22, 2018 10:11

GordonTheTurtle added the dco/no label Jan 22, 2018

xiaonancc77 force-pushed the master branch from dd6becc to 28eed83 Compare January 23, 2018 06:56

GordonTheTurtle removed the dco/no label Jan 23, 2018

xiaonancc77 force-pushed the master branch 2 times, most recently from 1e5d5d1 to 9f6c42f Compare January 23, 2018 08:33

GordonTheTurtle added the dco/no label Jan 23, 2018

xiaonancc77 force-pushed the master branch from 23aa96a to 61ad21e Compare January 23, 2018 09:37

GordonTheTurtle added dco/no and removed dco/no labels Jan 23, 2018

xiaonancc77 force-pushed the master branch from 66221c6 to b9b4bc8 Compare January 24, 2018 07:04

GordonTheTurtle added the dco/no label Jan 24, 2018

xiaonancc77 force-pushed the master branch from b9b4bc8 to 3ec60f4 Compare January 24, 2018 07:08

GordonTheTurtle added the dco/no label Jan 24, 2018

xiaonancc77 force-pushed the master branch from 3ec60f4 to 0214ef6 Compare January 24, 2018 09:07

GordonTheTurtle removed the dco/no label Jan 24, 2018

stevvooe reviewed Feb 3, 2018

View reviewed changes

xiaonancc77 force-pushed the master branch from 0214ef6 to 248bd55 Compare February 27, 2018 13:25

nwt reviewed Mar 9, 2018

View reviewed changes

nwt reviewed Mar 10, 2018

View reviewed changes

Added ignore event typs into notifications

fc1d364

Signed-off-by: elsanli(李楠) <[email protected]>

xiaonancc77 force-pushed the master branch from 248bd55 to fc1d364 Compare March 13, 2018 08:02

xiaonancc77 changed the title ~~Added ignore event typs into notifications~~ Added ignore event types into notifications Mar 14, 2018

dmcgowan merged commit 607ae5d into distribution:master Mar 14, 2018

klbjlabs mentioned this pull request May 24, 2018

support ignore event in notifications goharbor/harbor#5035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ignore event types into notifications #2501

Added ignore event types into notifications #2501

xiaonancc77 commented Jan 18, 2018

GordonTheTurtle commented Jan 18, 2018

codecov bot commented Jan 18, 2018 •

edited

Loading

stevvooe Jan 18, 2018

xiaonancc77 Jan 22, 2018

stevvooe commented Jan 18, 2018

xiaonancc77 commented Jan 22, 2018

xiaonancc77 commented Jan 24, 2018

xiaonancc77 commented Jan 26, 2018

stevvooe commented Feb 3, 2018

stevvooe Feb 3, 2018

stevvooe Feb 3, 2018

stevvooe Feb 3, 2018

stevvooe Feb 3, 2018

xiaonancc77 Feb 6, 2018

stevvooe commented Feb 3, 2018

nwt commented Feb 6, 2018

xiaonancc77 commented Feb 27, 2018

stevvooe commented Mar 9, 2018

nwt left a comment

nwt Mar 9, 2018

nwt Mar 9, 2018

xiaonancc77 Mar 13, 2018

xiaonancc77 commented Mar 13, 2018

nwt commented Mar 13, 2018

dmcgowan commented Mar 14, 2018

Added ignore event types into notifications #2501

Added ignore event types into notifications #2501

Conversation

xiaonancc77 commented Jan 18, 2018

GordonTheTurtle commented Jan 18, 2018

codecov bot commented Jan 18, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Jan 18, 2018

xiaonancc77 commented Jan 22, 2018

xiaonancc77 commented Jan 24, 2018

xiaonancc77 commented Jan 26, 2018

stevvooe commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Feb 3, 2018

nwt commented Feb 6, 2018

xiaonancc77 commented Feb 27, 2018

stevvooe commented Mar 9, 2018

nwt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaonancc77 commented Mar 13, 2018

nwt commented Mar 13, 2018

dmcgowan commented Mar 14, 2018

codecov bot commented Jan 18, 2018 •

edited

Loading