-
Notifications
You must be signed in to change notification settings - Fork 640
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(plugins): Add remote extensions field to a plugin info release #917
feat(plugins): Add remote extensions field to a plugin info release #917
Conversation
use = JsonTypeInfo.Id.NAME, | ||
include = JsonTypeInfo.As.EXTERNAL_PROPERTY, | ||
property = "type") | ||
@JsonSubTypes({@JsonSubTypes.Type(value = StageRemoteExtensionConfig.class, name = "stage")}) |
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.
stage
here corresponds to the RemoteExtension#type
field.
@Nonnull private String url; | ||
|
||
/** A placeholder for misc. configuration for the underlying HTTP client. */ | ||
@Nonnull private Map<String, Object> config = new HashMap<>(); |
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 can be flushed out over time, but for now it's a catch-all for any underlying client configuration we need.
@Nonnull private String description; | ||
|
||
/** Map of stage parameter names and default values. */ | ||
@Nonnull private Map<String, Object> parameters = new HashMap<>(); |
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 for now a map of the parameter name and the default value will suffice.
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.
Do like.
PluginInfo
is @Valid
, so I think it'd be worth adding javax.validation
annotations to some of these properties (such as @URL
on the url
property, etc).
...0-core/src/main/java/com/netflix/spinnaker/front50/model/plugins/remote/RemoteExtension.java
Outdated
Show resolved
Hide resolved
...0-core/src/main/java/com/netflix/spinnaker/front50/model/plugins/remote/RemoteExtension.java
Outdated
Show resolved
Hide resolved
public RemoteExtensionConfig config; | ||
|
||
/** Root remote extension configuration type. */ | ||
public interface RemoteExtensionConfig {} |
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.
Rather than define the extension point config type in front50 (making front50 changes required for most remote extensions), I think we should investigate a shared model or service-exposed schema for extension point configs - personally I favor the schema idea:
For any extension point that has a config, say MyRemoteExtensionConfig
, we could convert to JSON Schema (or similar) and expose it from something like /plugins/extension-point-configs/
. Each config would have a type
, as you have it, but structured like {service}/{configType}[@version?]
[1], allowing front50 to lookup the correct schema when a remote plugin is being published and we want to do some type / constraint validation. This would let us continue to define, in a single place, everything for an extension point.
I don't think we need to change the PR as it exists today, since this is talking about a lot of work - we can always retrofit what you have here after we've got something working end-to-end. 💯
[1] Not sure if version is needed, but probably? 🤷
One problem with this is that services tend to have dependencies on front50, but not the other way around. I think I'd also be OK with services publishing their schemas into front50 on startup (either via HTTP or pubsub - prefer pubsub). 🤷
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.
Super interesting idea.
I think a version would be needed. Each change to the config schema would need to be versioned, and then if there is a new config schema version a new plugin info release version with the new config schema would need to be published into front50. This is because we will be rolling out remote extensions by version, and we can't really swap out the underlying config schema without some corresponding version bump.
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.
What would schema validation failures gate? Just publishes of new plugins?
In spinnaker/orca#3840 I realized I had the cart before the horse on a few items, specifically the basic remote extension point configuration. This will not only allow me to get away from abusing the PreconfiguredJob stuff even more (by allowing for specific remote stage configuration), but also lay some of the ground work for other types of remote extension points.
Adds a
remoteExtensions
field to a plugin inforelease
. A plugin that is just a remote extension look like this:The idea here is that we use the
requires
field in-conjunction with the remote extensiontype
to determine which service to configure with the specific remote extension and corresponding remote extension configuration.