Add base data stream into package #681
Conversation
There was a problem hiding this comment.
TL;DR Remember, the spec goes first :)
There are few steps to push this change:
- Package-spec.
- API design of package-registry.
- Implementation of package-registry.
Re 1:
I think we all agree with expand_data_streams. Do you want to introduce images/screenshots on the data stream level? You can open a PR to package-spec and we can discuss it there.
Re 2:
I would consider two approaches:
- Introduce data streams previews (
BaseDataStream) which contain as min information as possible comparing to the owning package.
Re 3:
Once we agree on 1. and 2., this is a good moment to go with implementation of the package-registry.
| @@ -55,6 +58,17 @@ type DataStream struct { | |||
| BasePath string `json:"-" yaml:"-"` | |||
| } | |||
|
|
|||
| type BaseDataStream struct { | |||
There was a problem hiding this comment.
I was wondering why is it a "base" data stream. I got the idea, but can't relate this name with meaning.
| @@ -97,19 +111,19 @@ type fieldEntry struct { | |||
| aType string | |||
| } | |||
|
|
|||
| func NewDataStream(basePath string, p *Package) (*DataStream, error) { | |||
| func NewDataStream(basePath string, p *Package) (*DataStream, *BaseDataStream, error) { | |||
There was a problem hiding this comment.
The util package contains structs and methods which can be used in other repositories. I woudn't change signatures of exported methods.
| @@ -42,6 +42,9 @@ type DataStream struct { | |||
| Title string `config:"title" json:"title" validate:"required"` | |||
| Release string `config:"release" json:"release"` | |||
|
|
|||
| Description string `config:"description" json:"description"` | |||
There was a problem hiding this comment.
In general BasePackage and DataStream correspond to their format in package-spec. I wouldn't extend them this way. If you need to add more fields to view, but not manifest, I would consider creating new structures (possibly extending ones in spec).
| Type string `config:"type" json:"type" validate:"required"` | ||
| Dataset string `config:"dataset" json:"dataset,omitempty" yaml:"dataset,omitempty"` |
There was a problem hiding this comment.
Looks like redundant information?
| Path string `json:"path" yaml:"path,omitempty"` | ||
| Icons []Image `config:"icons,omitempty" json:"icons,omitempty" yaml:"icons,omitempty"` | ||
| Internal bool `config:"internal,omitempty" json:"internal,omitempty" yaml:"internal,omitempty"` | ||
| BaseDataStreams []*BaseDataStream `config:"base_data_streams,omitempty" json:"base_data_streams,omitempty" yaml:"base_data_streams,omitempty"` |
There was a problem hiding this comment.
BaseDataStream contains redundant data that you can find in package. I think we should keep them only in package to prevent the response body from increasing.
BTW base data streams look to me more like data stream previews or sth (just looking for a different name :)
(just a draft PR for my small poc)
This PR is to adjust the code to read the new config option
expand_data_streamat the package level. Whenexpand_data_streamis set to true, abstract data stream info and store them into the/searchAPI endpoint result.For example with
expand_data_stream: truein AWS package,/searchAPI response will look like this: