Add mongodb client #653
Add mongodb client #653
Conversation
pkg/model/bson.go
Outdated
| func (a *Application) MarshalBSON() ([]byte, error) { | ||
| // TODO: Replace "fatih/structs" with "mitchellh/mapstructure" | ||
| // Currently using fatih/structs temporarily to easily convert all nested structures into a map. | ||
| m := structs.Map(a) |
There was a problem hiding this comment.
All of the nested structs can be converted to maps. So keys are ensured to be camel case like:
{ "_id" : "abc123", "Name" : "foo", "ProjectId" : "proj-id", "CloudProvider" : "provider", "CreatedAt" : NumberLong(1), "UpdatedAt" : NumberLong(1), "EnvId" : "env-id", "MostRecentlySuccessfulDeployment" : null, "Kind" : 0, "MostRecentlyTriggeredDeployment" : null, "SyncState" : { "Status" : 1, "ShortReason" : "", "Reason" : "", "HeadDeploymentId" : "", "Timestamp" : NumberLong(1) }, "Disabled" : false, "PipedId" : "piped-id", "GitPath" : { "ConfigPath" : "", "ConfigFilename" : "", "Url" : "", "Repo" : { "Id" : "id", "Remote" : "", "Branch" : "" }, "Path" : "path" } }
|
/cc @stormcat24 @nghialv |
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
I've tested all of methods of |
|
|
||
| func NewMongoDB(ctx context.Context, url, database string, opts ...Option) (*MongoDB, error) { | ||
| // TODO: Enable to specify username and password via file. | ||
| // Need to check if it overrides AuthMechanism etc. |
There was a problem hiding this comment.
Just a question, why the second line has a different indent? The TODO plugin will do something for us?
There was a problem hiding this comment.
Sorry for letting you get confused. This is for IntelliJ IDEA. Making indent, then IDEA can recognize they are part of TODO comment and highlights:
pkg/model/bson.go
Outdated
| m[mongodbPrimaryKey] = a.Id | ||
| delete(m, "Id") | ||
|
|
||
| return bson.Marshal(m) |
There was a problem hiding this comment.
In this way, we cost 2 times for converting the model to the map and marshaling the map.
How about using a wrapper struct to minimize the cost.
type IDGetter interface {
GetID() string
}
type MongoDBModelWrapper struct {
IDGetter `bson:",inline"`
ID objectid.ObjectID `bson:"_id"`
}
// The IDGetter can be any model: Application, Deployment, ...
There was a problem hiding this comment.
I'm looking into that way.
There was a problem hiding this comment.
Although this way causes adding both _id and id fields inside the DB, I think that is fine.
There was a problem hiding this comment.
I've been considering that.
|
Unmarshaling bson requires a concrete type, that's why I determined to use concrete type structs, not interfaces suggested at #653 (comment) |
|
Code coverage for golang is
|
| @@ -0,0 +1,186 @@ | |||
| // Copyright 2020 The PipeCD Authors. | |||
There was a problem hiding this comment.
I'm thinking about moving the wrapper implementation to a new mongodb/wrapper package.
There was a problem hiding this comment.
All of them are private models and functions so I think it is fine to place them inside this package.
| setID(id string) | ||
| // storeModel stores the unwrapped model in the value pointed to by v. | ||
| storeModel(v interface{}) error | ||
| } |
There was a problem hiding this comment.
Because eventually, we have to define all of the wrappers (application, command...) in this package so just want to simplify this part.
func wrapModel(entity interface{}) (interface{}, error) {
switch e := entity.(type) {
case *model.Application:
return *application{Application: e, ID: e.Id}, nil
....
default:
return nil, fmt.Errorf("%T is not supported", e)
}
}
func extractModel(w interface{}, e interface{}) error {
switch w:= w.(type) {
case *application:
*e = w.Application
...
default:
return nil, fmt.Errorf("%T is not supported", e)
}
}
There was a problem hiding this comment.
Agree with your idea, gonna apply it.
There was a problem hiding this comment.
Hmm... actually we need setID method for updating. MongoDB doesn't allow empty "_id" when updating.
There was a problem hiding this comment.
Not sure why do need that setID method. The _id value was already set in the wrapModel function.
There was a problem hiding this comment.
Ok, take a look at the behavior of updating. The given entity doesn't have Id, so the generated wrapper has no way to know its id.
https://github.com/pipe-cd/pipe/pull/653/files/bb7bc703dbd015ab851faee9501d86797b160e08#diff-825491436922437713c5651bc145ccd8R189-R212
There was a problem hiding this comment.
Hold on, id shoud be populated when decoding.
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Enable to specify username and password via file.This was created by todo plugin since "TODO:" was found in 1dc024c when #653 was merged. cc: @nakabonne.2. Support updating process with using transaction on mongoDB clusterThis was created by todo plugin since "TODO:" was found in 1dc024c when #653 was merged. cc: @nakabonne. |
|
Applied 👍 |
|
Good Job! |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
|
Thank you for your deep review. |
|
failed to handle template rule examples Details |
What this PR does / why we need it:
With this PR, MongoDB will be available only on a standalone server as a datastore for control-plane.
Which issue(s) this PR fixes:
Fixes #37
Does this PR introduce a user-facing change?: