feat(core): Adding pipeline template storage #222
Conversation
| @SuppressWarnings("unchecked") | ||
| public List<String> getScope() { | ||
| Map<String, Object> metadata = (Map<String, Object>) super.get("metadata"); | ||
| if (metadata.isEmpty()) { |
|
|
||
| @JsonIgnore | ||
| @SuppressWarnings("unchecked") | ||
| public List<String> getScope() { |
There was a problem hiding this comment.
scope vs scopes?
I suspect there's some motivation to the naming ... I just momentarily got caught up scope implying singular and it returning a list.
There was a problem hiding this comment.
I debated on this a little bit, then decided to just throw something out there. I suspect that most people will scope templates to a single item (global or a specific app), which is why I chose the singular. Definitely happy to make it plural, since it is a list.
|
|
||
| public boolean containsAnyScope(List<String> scope) { | ||
| for (String s : scope) { | ||
| if (getScope().contains(s)) { |
There was a problem hiding this comment.
Any worry about case sensitivity wrt to scopes?
| @@ -166,6 +169,11 @@ public PipelineDAO pipelineDAO(GcsStorageService service, Registry registry) { | |||
| } | |||
|
|
|||
| @Bean | |||
| public PipelineTemplateDAO pipelineTemplateDAO(StorageService storageService, Registry registry) { | |||
| return new DefaultPipelineTemplateDAO(storageService, Schedulers.from(Executors.newFixedThreadPool(25)), PIPELINE_TEMPLATE_REFRESH_MS, registry); | |||
There was a problem hiding this comment.
Perhaps too many threads (depends on how many # of pipeline templates and concurrent calls to getByScope) but I've got a PR in flight that lets this be a little more configurable.
| @@ -138,6 +140,11 @@ public PipelineDAO pipelineDAO(StorageService storageService, Registry registry) | |||
| } | |||
|
|
|||
| @Bean | |||
| public PipelineTemplateDAO pipelineTemplateDAO(StorageService storageService, Registry registry) { | |||
| return new DefaultPipelineTemplateDAO(storageService, Schedulers.from(Executors.newFixedThreadPool(25)), 10000, registry); | |||
There was a problem hiding this comment.
Technically you've got a different refresh interval between S3 and GCS ... my PR will clean this up as well but I'd suggest dropping it down to 60s.
Any call to all() will do an on-demand refresh of any deltas anyways so it's more about having a reasonably fresh cache in the event of a hystrix fallback.
|
|
||
| @ExceptionHandler(InvalidPipelineTemplateRequestException.class) | ||
| @ResponseStatus(HttpStatus.BAD_REQUEST) | ||
| Map handleInvalidPipelineTemplateRequestException(InvalidPipelineTemplateRequestException e) { |
There was a problem hiding this comment.
Nothing to do with this PR but I reckon we can do a much better job of exception handling that doesn't require per-controller exception classes ...
There was a problem hiding this comment.
Agree, I'm still getting my bearings on how we do error handling in general in the services. I don't want to run off doing something different until I've got a good handle on how to do it better.
There was a problem hiding this comment.
I think it's completely reasonable to do it this way in front50 for now ... and then one of us can do a collective refactor across the board when we've got a more reasonable pattern.
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class PipelineTemplate extends HashMap<String, Object> implements Timestamped { |
There was a problem hiding this comment.
Doesn't have to extends HashMap<> if it makes more sense to do a concrete object w/ @JsonAnyGetter/JsonAnySetter for the dynamic pipeline bits.
Would let you concretely model the metadata but meh?
|
I don't see anything out of the ordinary, lgtm (few comments above but nothing blocking I don't think). |
robzienert
force-pushed
the
pipelinetemplate-storage
branch
2 times, most recently
from
8f38884
to
e793428
Compare
| @@ -28,6 +29,7 @@ | |||
| PROJECT(Project.class, "projects", "project-metadata.json"), | |||
| PIPELINE(Pipeline.class, "pipelines", "pipeline-metadata.json"), | |||
| STRATEGY(Pipeline.class, "pipeline-strategies", "pipeline-strategy-metadata.json"), | |||
| PIPELINE_TEMPLATE(PipelineTemplate.class, "pipelineTemplates", "pipeline-template-metadata.json"), | |||
There was a problem hiding this comment.
maybe pipeline-templates vs pipelineTemplates to be consistent with the naming convention of other types.
robzienert
force-pushed
the
pipelinetemplate-storage
branch
2 times, most recently
from
47a16f0
to
8dc6897
Compare
robzienert
force-pushed
the
pipelinetemplate-storage
branch
from
8dc6897
to
c6cecc7
Compare
The whole Redis
findByIditerating and filteringall()pattern feels wrong, and thescopeinteraction seems janky, but interested in initial feedback.@spinnaker/netflix-reviewers PTAL