Kick off data sources service #5022
Conversation
JAORMX
force-pushed
the
datasourceservice
branch
2 times, most recently
from
5e93fb8 to
cb82429
Compare
JAORMX
force-pushed
the
datasourceservice
branch
5 times, most recently
from
5d646db to
f63c683
Compare
|
@blkt @teodor-yanev and I talked about this and agreed to include the project ID in the functions and relationships tables. @blkt will bring this in and I'll rebase on top of it. |
JAORMX
force-pushed
the
datasourceservice
branch
2 times, most recently
from
b4643aa to
23885bf
Compare
This service will contain all the business logic to deal with data sources. Signed-off-by: Juan Antonio Osorio <[email protected]>
JAORMX
force-pushed
the
datasourceservice
branch
from
23885bf to
bea1ced
Compare
JAORMX
requested review from
jhrozek,
eleftherias,
rdimitrov and
teodor-yanev
| @@ -0,0 +1,73 @@ | |||
| -- CreateDataSource creates a new datasource in a given project. | |||
There was a problem hiding this comment.
Should we have delete on cascade based on the data source id for functions?
There was a problem hiding this comment.
| } | ||
|
|
||
| // All data source types should be equal... so we'll just take the first one. | ||
| dsfType := dsfuncs[0].Type |
There was a problem hiding this comment.
Should we ensure/validate that all sources have the same type?
There was a problem hiding this comment.
We should. I was thinking of doing this on the create and update parts though. There's not much we can do on the get side.
| }) | ||
| if err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return db.DataSource{}, util.UserVisibleError(codes.NotFound, |
There was a problem hiding this comment.
Isn't it better to return nil instead of empty object in case of errors?
Comment is valid for the other use cases below
There was a problem hiding this comment.
this sub-function returns an object and not a pointer, since that's the type of object that the database wrappers usually returns. we do return pointers and thus nil underneath though.
|
|
||
| func (d *dataSourceService) List( | ||
| ctx context.Context, project uuid.UUID, opts *ReadOptions) ([]*minderv1.DataSource, error) { | ||
| stx, err := d.txBuilder(d, opts) |
There was a problem hiding this comment.
Question: Why do we want to list within a transaction?
There was a problem hiding this comment.
So, data sources (and their functions) are part of separate tables, we want the states of these to be consistent when listing/getting them, and not get into funky intermediate states if parallel update happens.
Note that I do set the transaction to be "read-only" so this doesn't block other reads.
Summary
This service will contain all the business logic to deal with data
sources.
For the moment this only kicks off the
getcalls. But populating this shouldallow others to work on other function definitions.
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: