Skip to content
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

Add pluggable repository layer #39

Conversation

hiepd
Copy link
Collaborator

@hiepd hiepd commented Jul 18, 2020

Repository consists of

  • generic repository
  • nosql repository

Part of #34

Usage:

import (
  "context"
  "github.com/theycallmemac/odin/odin-engine/pkg/repository"
  _ "github.com/theycallmemac/odin/odin-engine/pkg/repository/all"
)

repo, err := repository.GetRegistration("mongodb").OpenFunc("localhost:27017", nil)

repo.GetJobStats(context.Background(), "123")

With odin-engine

./odin-engine -storage_name=mongodb -storage_address=localhost:27017

@hiepd hiepd marked this pull request as draft July 18, 2020 04:56
@hiepd hiepd force-pushed the feature/34_repository_layer branch from 6156b20 to f7ddf05 Compare July 18, 2020 04:59
@theycallmemac theycallmemac self-requested a review July 18, 2020 15:21
Copy link
Owner

@theycallmemac theycallmemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks quite good so far. Just worth mentioning that along with the observability table there is also a jobs table.

@theycallmemac
Copy link
Owner

@hiepd the build is failing because github.com/theycallmemac/odin/odin-engine/pkg/repository/nosql doesn't exist in the master branch yet. That's to be expected right now.

@hiepd hiepd force-pushed the feature/34_repository_layer branch from f7ddf05 to 7f1cf53 Compare July 25, 2020 07:23
@hiepd hiepd marked this pull request as ready for review July 25, 2020 07:25
@hiepd hiepd changed the title [WIP] Add pluggable repository layer Add pluggable repository layer Jul 25, 2020
Copy link
Owner

@theycallmemac theycallmemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some Github Actions failing on this, but I'm not too worried about this right now. There's likely some logic here and there that needs some changes.

I see you specify the datastore when you run the odin-engine binary. It could potentially be better for the user to define their datastore details in https://github.com/theycallmemac/odin/blob/master/odin-engine/config/odin-config.yml.

@hiepd hiepd force-pushed the feature/34_repository_layer branch from 7f1cf53 to 183ded2 Compare July 26, 2020 11:29
@hiepd hiepd force-pushed the feature/34_repository_layer branch from 183ded2 to 4f715f6 Compare July 26, 2020 12:07
@hiepd hiepd requested a review from theycallmemac July 26, 2020 12:09
@hiepd
Copy link
Collaborator Author

hiepd commented Jul 26, 2020

Thanks @theycallmemac . I have changed the code to use config-odin.yml like requested. There are some formatting changes in api/main.go as well since the file wasn't formatted.

I think we can also figure out how to run migration for relational storage in the future as well.

Copy link
Owner

@theycallmemac theycallmemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay this is looking good as far as the changes are concerned. I'll take a look at the failing build and see if I can get it fixed!

@theycallmemac
Copy link
Owner

@hiepd Having a quick look. The dependency step is breaking because /pkg/repository doesn't exist in the master branch, once this is merged it should fix it.

@theycallmemac theycallmemac changed the base branch from master to v2.0.0/pluggable-repository-layer July 28, 2020 13:38
@theycallmemac theycallmemac merged commit 8195778 into theycallmemac:v2.0.0/pluggable-repository-layer Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants