-
Notifications
You must be signed in to change notification settings - Fork 638
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(plugin-registry): Front50 plugins storage service #625
Conversation
6fd29a0
to
2ef3985
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.
looks pretty good! couple minor comments/questions
front50-sql/src/main/kotlin/com/netflix/spinnaker/front50/model/SqlStorageService.kt
Outdated
Show resolved
Hide resolved
front50-sql/src/main/resources/db/changelog/20191030-initial-plugins-schema.yml
Outdated
Show resolved
Hide resolved
front50-core/src/main/groovy/com/netflix/spinnaker/front50/model/plugin/Plugin.java
Show resolved
Hide resolved
2ef3985
to
821b2b3
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.
Do any of the other storage implementations besides SQL need to be updated to make this work? (I think no..)
I suspect we want a ConditionalOnProperty as to whether to expose this controller (I know we did that most recently on the DeliveryController
...core/src/main/groovy/com/netflix/spinnaker/front50/model/plugin/DefaultPluginRepository.java
Outdated
Show resolved
Hide resolved
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 LGTM - I just had some minor other comments
From what I can tell the non-SQL implementations should just work. I'll add a |
821b2b3
to
fd8dd36
Compare
front50-sql/src/main/resources/db/changelog/20191030-initial-plugins-schema.yml
Outdated
Show resolved
Hide resolved
fd8dd36
to
e67e72d
Compare
Is this necessary? It may be too soon (or not necessary) to add this functionality to Front50, or it may be that this manifest can be greatly simplified. |
Closing for now. |
Plain ol' JSON common storage service front50 implementation. Protobuf needs a bit of extra work to get it working with this implementation.
Previous PR: #621