-
Notifications
You must be signed in to change notification settings - Fork 657
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(keel): send keel docker events if enabled #535
Conversation
ca9c973
to
beee95c
Compare
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.
LGTM, just had a small question on (possibly?) unauthenticated requests to keel.
payload: [artifacts: [artifact], details: [:]], | ||
eventName: "spinnaker_artifacts_docker" | ||
] | ||
AuthenticatedRequest.allowAnonymous { keelService.get().sendArtifactEvent(artifactEvent) } |
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.
Is the allowAnonymous
bit common practice/OK?
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.
It's common for service to service calls, i.e. we're not doing this call on behalf of a user so we have no user auth info to pass on.
P.S. Sorry if this is a silly question, but if we were already sending an event to echo, and echo centralizes most event handling to/from other services, shouldn鈥檛 we just extend the event handler in echo to call keel? |
@luispollo that's a good question. Since the information is coming from |
BTW @emjburns I noticed that this echo PR from earlier this year introduced echo -> keel integration for artifact events. How does that relate to/differ from what we're trying to accomplish here? |
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.
LGTM
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.
Two small comments, otherwise looks good!
igor-web/src/main/java/com/netflix/spinnaker/igor/config/KeelConfig.java
Outdated
Show resolved
Hide resolved
igor-web/src/main/groovy/com/netflix/spinnaker/igor/docker/DockerMonitor.groovy
Outdated
Show resolved
Hide resolved
(answered Luis's questions offline) |
2c9b7e3
to
72ded70
Compare
In order to react to new docker containers in
keel
we need to know when there are new containers. This PR adds functionality to emit events tokeel
with the same criteria as sending new events to echo.This won't kick in unless you have keel enabled. The format for the docker event we send is subject to change, but I think this is a decent first start 馃し鈥嶁檧 we shall see.