-
Notifications
You must be signed in to change notification settings - Fork 740
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
feat(gremlin): Add proxy of Gremlin API to retrieve command and target templates #725
Conversation
We prefer that non-test backend code be written in Java or Kotlin, rather than Groovy. The following files have been added and written in Groovy:
See our server-side commit conventions here. |
@@ -35,7 +35,8 @@ services: | |||
packageType: RPM | |||
# optional services: | |||
echo: | |||
enabled: false | |||
baseUrl: http://localhost:8089 | |||
enabled: true |
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.
Why does this PR modify Echo configurations?
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 believe it's to improve the local development story ... do these configs get used for anything other than running locally? Does halyard use them @ezimanyi? (we don't use these configs internally so a bit out of the loop).
FWIW echo
is no longer an optional service as pipeline triggering flows through it.
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.
@ajordens : Halyard does not use these configs. Halyard uses the ones in the halconfig
directory of each service as a base, which it then templates on the settings generated from your halyard config, but doesn't use the ones in service-web/config/...
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 thought that's how it worked but thanks for clarifying @ezimanyi!
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.
When I run Gate without Echo being enabled, and execute a pipeline, I get the following stacktrace:
2019-02-19 10:28:35.671 ERROR 5815 --- [nio-8084-exec-4] c.n.s.g.controllers.PipelineController : Unable to trigger pipeline (application: foo, pipelineId: NewTemplate)
java.lang.NullPointerException: Cannot invoke method postEvent() on null object
at org.codehaus.groovy.runtime.NullObject.invokeMethod(NullObject.java:91) ~[groovy-all-2.4.15.jar:2.4.15]
at org.codehaus.groovy.runtime.callsite.PogoMetaClassSite.call(PogoMetaClassSite.java:47) ~[groovy-all-2.4.15.jar:2.4.15]
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47) ~[groovy-all-2.4.15.jar:2.4.15]
at org.codehaus.groovy.runtime.callsite.NullCallSite.call(NullCallSite.java:34) ~[groovy-all-2.4.15.jar:2.4.15]
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47) ~[groovy-all-2.4.15.jar:2.4.15]
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:116) ~[groovy-all-2.4.15.jar:2.4.15]
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:128) ~[groovy-all-2.4.15.jar:2.4.15]
at com.netflix.spinnaker.gate.services.PipelineService.triggerViaEcho(PipelineService.groovy:114) ~[main/:na]
I had forgotten this was included in this PR. If it makes your life easier, I'm happy to submit this as a separate PR, and also move the echo
config out of the optional block in gate.yml
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.
Fixing the local dev config seems alright to me.
gate-web/config/gate.yml
Outdated
@@ -48,6 +49,9 @@ services: | |||
enabled: false | |||
canaryConfigStore: true | |||
baseUrl: http://localhost:8090 | |||
gremlin: | |||
enabled: true |
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 doesn't seem correct that Gremlin would default to on
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 suggest a new block:
integrations:
gremlin:
baseUrl: https://api.gremlin.com/v1
enabled: true|false
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.
Makes sense @ajordens and thanks for the review @avram . Is the following what you have in mind?
- Create a new block
integrations
ingate.yml
- Move
GremlinController
andGremlinService
into a new top-level modulegate-integrations-gremlin
. - Fix the dependency injection to respect the
integrations.gremlin.enabled
flag and only instantiateGremlinController
andGremlinService
if that flag is true.
I'm guessing similar thoughts apply to the Gremlin work in orca
? I'm not as familiar with how we can separate out the UI changes in deck
- any thoughts on how that should get modularized? In the meantime, will start working through the gate
+ orca
changes.
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 think there's an opportunity here to define how external integrations would fit in.
I'd suggest splitting out the grelim-specific code into a new gate-integrations-gremlin
artifact rather than putting code into gate-web
.
This would provide a better story around ongoing review/maintenance. I'd like to be able to say, Gremlin folks own gate-integrations-gremlin
and I'm generally ok with what goes in there vs. having to worry about changes in a core artifact like gate-web
.
@@ -35,7 +35,8 @@ services: | |||
packageType: RPM | |||
# optional services: | |||
echo: | |||
enabled: false | |||
baseUrl: http://localhost:8089 | |||
enabled: true |
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 believe it's to improve the local development story ... do these configs get used for anything other than running locally? Does halyard use them @ezimanyi? (we don't use these configs internally so a bit out of the loop).
FWIW echo
is no longer an optional service as pipeline triggering flows through it.
gate-web/config/gate.yml
Outdated
@@ -48,6 +49,9 @@ services: | |||
enabled: false | |||
canaryConfigStore: true | |||
baseUrl: http://localhost:8090 | |||
gremlin: | |||
enabled: true |
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 suggest a new block:
integrations:
gremlin:
baseUrl: https://api.gremlin.com/v1
enabled: true|false
import org.springframework.web.bind.annotation.RestController | ||
|
||
@RestController | ||
@RequestMapping('/gremlin') |
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.
This also needs to be made conditional on gremlin being enabled, otherwise startup will fail trying to autowire the missing GremlinService
.
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.
These PRs are ready to merge after the suggest config
and code locations are updated.
@@ -35,7 +35,8 @@ services: | |||
packageType: RPM | |||
# optional services: | |||
echo: | |||
enabled: false | |||
baseUrl: http://localhost:8089 | |||
enabled: true |
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 thought that's how it worked but thanks for clarifying @ezimanyi!
… wired up the enabled flag to DI
@@ -45,6 +46,6 @@ class ServiceConfiguration { | |||
} | |||
|
|||
Service getService(String name) { | |||
services[name] | |||
(services + integrations)[name] |
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 kept the integrations
structure the same as services
in gate.yml
so that this code works
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.
👍
createClient('gremlin', GremlinService, okHttpClient) | ||
} | ||
|
||
private <T> T createClient(String serviceName, |
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.
These helper methods are copied from GateConfig
. A more DRY approach would be to have some abstracted-out code that both GateConfig
and GremlinGateConfig
depend on, but I wanted to get your input before I attempt to refactor one of the core classes
|
||
@RestController | ||
@RequestMapping('/gremlin') | ||
@ConditionalOnExpression('${integrations.gremlin.enabled:true}') |
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.
this is new, and I've tested it. only other change in this file is the package (addition of gremlin
)
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.
Won't this default it to enabled (whereas the bean will default to disabled)?
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.
When I tested this locally, setting the value of the property in gate.yml
to false
did not inject this RequestMapping
. When the UI made the call, it 404ed, as expected.
When I set it to true
, the endpoint was properly mapped and made the appropriate redirection to the Gremlin API.
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'm referring to the case where the config setting is absent entirely---if you don't have the integrations.gremlin.enabled
key in your environment at all. Then:
- the bean checks
@ConditionalOnProperty('integrations.gremlin.enabled')
and does not get created - the controller checks
@ConditionalOnExpression('${integrations.gremlin.enabled:true}')
, falls back to the default oftrue
in the expression, and gets created.
Then the controller tries to autowire the bean, and gate fails to start up.
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 misunderstood the syntax of the annotation - thanks for the walkthrough! Changed as you suggested and manually tested all three variants: true
/ false
/ missing.
@@ -21,6 +21,7 @@ repositories { | |||
dependencies { | |||
compile project(":gate-core") | |||
compile project(":gate-proxy") | |||
compile project(":gate-integrations-gremlin") |
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 didn't see any way around this. I need the @Configuration
of GremlinGateConfig
to get detected, so I need those classes visible to the webapp.
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.
👍 Yes, there's no way around this right now.
Subsequent integrations we could probably wire something up that would automatically add dependencies on anything matching a *integrations*
pattern.
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.
Adjust the endpoint and it looks mergeable to me.
@@ -45,6 +46,6 @@ class ServiceConfiguration { | |||
} | |||
|
|||
Service getService(String name) { | |||
services[name] | |||
(services + integrations)[name] |
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.
👍
import org.springframework.web.bind.annotation.RestController | ||
|
||
@RestController | ||
@RequestMapping('/gremlin') |
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.
Lets make this /integrations/gremlin
@@ -35,7 +35,8 @@ services: | |||
packageType: RPM | |||
# optional services: | |||
echo: | |||
enabled: false | |||
baseUrl: http://localhost:8089 | |||
enabled: true |
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.
Fixing the local dev config seems alright to me.
Move the `GremlinController` endpoints to be rooted at `/integrations/gremlin`. `integrations.gremlin.enabled` is `false` when running in local development mode.
@mattrjacobs I pushed some changes to your branch that did a bit of cleanup + groovy -> java conversion ... if it looks good to you I'll merge. |
LGTM @ajordens - thanks for doing the conversion. I submitted a PR to deck to adjust the Gate endpoint here: spinnaker/deck#6591. Will begin work to add the integrations pattern to orca as well. |
Necessary for Deck change: spinnaker/deck#6549
Also related to Orca change: spinnaker/orca#2664