-
Notifications
You must be signed in to change notification settings - Fork 7k
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
First set of changes for persistent repository. #170
Conversation
@vaikas-google updated to skip the tests if mongodb is not running. Now ready to go. PTAL |
Could the persistence layer made pluggable by using an interface? I don't have anything against mongodb but it isn't in our current stack so I'd rather not have to care about it. Apologies if it isn't the right forum to discuss this, but I was curious about why we picked mongodb and not make it pluggable like following the |
@jtblin: it is already an interface. See repository/repository.go. There are currently two implementations, in-memory and persistent. The latter is MongoDB. Should be easy enough to add another. I suppose we could provide a repository plug-in API to make it a little cleaner. Can you open an issue for it? |
ok I see. Sorry about that, I just looked quickly at the diff and it was looking like we were just adding mongodb in |
811d3da
to
0dd5af3
Compare
"gopkg.in/mgo.v2/bson" | ||
) | ||
|
||
type pDeployment struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just deployment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a persistent deployment. There's a transient deployment named tDeployment in transient.go. Yes, they're in different packages, so they could be the same, but given that we're defining types that differ significantly, I thought it better to give them different names.
@technosophos PTAL |
@@ -0,0 +1,325 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a rename for the existent repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We have both in service. Manager falls back the transient repository if the persistent one is not available.
@vaikas-google Can I get an LGTM? |
LGTM |
First set of changes for persistent repository.
Fixes #52. |
Resolves helm#170
Signed-off-by: yxxhero <aiopsclub@163.com>
Initial PR for persistence based on MongoDB. Includes all code changes and tests correctly against a localhost instance of MongoDB. Automated tests fail because MongoDB is not running in the test environment. Next PR will automate setup of MongoDB service in the target cluster when dm is installed, and unblock unit tests.