Add fake metadata store and fix tests. #958
Conversation
Also, add instructions on how to build/run the backend with Bazel. Note that the fake metadata store works, but I need to add proper tests that exercise it. That'll be done in a separate PR. One thing I'm missing here is how to make Bazel run well in Travis. I will send a follow up PR for doing this.
| @@ -365,19 +365,26 @@ func (s *RunStore) UpdateRun(runID string, condition string, workflowRuntimeMani | |||
| // new in the status of an Argo manifest. This means we need to keep track | |||
| // manually here on what the previously updated state of the run is, to ensure | |||
| // we do not add duplicate metadata. Hence the locking below. | |||
| row := tx.QueryRow("SELECT WorkflowRuntimeManifest FROM run_details WHERE UUID = ? FOR UPDATE", runID) | |||
| var query string | |||
There was a problem hiding this comment.
Could we have an interface that abstracts away the differences between the MySQL and SQLLite DB? I think it would look cleaner than having switch statements.
There was a problem hiding this comment.
The difference in behaviour is real and not abstract, and in the interest of code readability, it's worth making this explicit. This makes it clear to the reader that with mysql, we do a row lock, while no such locking takes place with sqlite. That's a fundamental difference that I think is worth highlighting to the reader, rather than hide it behind some abstraction. It's crystal clear here that row locking path is never tested in unit tests as well.
There was a problem hiding this comment.
I moved this to the db interface, FWIW. PTAL.
| } | ||
|
|
||
| if s.metadataStore != nil { |
There was a problem hiding this comment.
Could we use the null design pattern here so that we don't forget to perform this check? (i.e. we have an interface with an actual implementation calling MySQL, and actual implementation calling SQLLite, and a "NullImplementation" that does not do anything.
This pattern really helps keep the code concise and easy to read by leveraging OO instead of requiring if/then/switch statement in several places.
There was a problem hiding this comment.
I removed the comparison to nil because I now expect s.metadataStore to always be non-nil, unlike before, when it was nil in tests.
Go is not an OO language, and I don't think it's a good practice to shoehorn OO design patterns in Go code.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vicaire The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vicaire The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test kubeflow-pipeline-e2e-test |
additionally remove some inactive reviewers
Also, add instructions on how to build/run the backend with Bazel.
Note that the fake metadata store works, but I need to add proper tests
that exercise it. That'll be done in a separate PR.
One thing I'm missing here is how to make Bazel run well in Travis. I
will send a follow up PR for doing this.
This change is