add rdbms support for cdn metadata storage #151
Conversation
| ) | ||
|
|
||
| // CdnMetadataEntry stores the module name and cdn URL. | ||
| type CdnMetadataEntry struct { |
There was a problem hiding this comment.
please, extract this into cdn/metadata and use in both mongo and rdbms
There was a problem hiding this comment.
will you update mongo in this pack as well? to use CdnMetadataEntry
pkg/cdn/metadata/rdbms/getter.go
Outdated
| if err := query.First(&result); err != nil { | ||
| return "", err | ||
| } | ||
| return result.RedirectURL, nil |
There was a problem hiding this comment.
possible to return result.RedirectURL, err? it should not be nil right
There was a problem hiding this comment.
Do you mean checkin if result == nil? If there is no row matching then First returns an error. Or did you mean something else?
There was a problem hiding this comment.
i meant replacing it with single return saying
err := query.First(&result)
return result.RedirectURL, errresult should not be nil so result.redirect uri should at least be empty string
|
|
||
| // String is not required by pop and may be deleted | ||
| func (e CdnMetadataEntry) String() string { | ||
| je, _ := json.Marshal(e) |
There was a problem hiding this comment.
m: maybe just log the error? it does not feel right to ignore
There was a problem hiding this comment.
It's the model code auto-generated by pop. Should I log this in the storage pkg as well?
There was a problem hiding this comment.
guess not, i did not realised that it is autogenerated
| ) | ||
|
|
||
| // CdnMetadataEntry stores the module name and cdn URL. | ||
| type CdnMetadataEntry struct { |
There was a problem hiding this comment.
@marpio can you name this CDNMetadataEntry?
pkg/cdn/metadata/mongo/mongo.go
Outdated
| // NewMongoStorage returns an unconnected Mongo backed storage | ||
| // that satisfies the Storage interface. You must call | ||
| // Connect() on the returned store before using it. | ||
| func NewMongoStorage(url, dbName string) *MetadataStore { |
There was a problem hiding this comment.
nit: this name "stutters" because it will be called with mongo.NewMongoStorage. can you call it NewStorage instead?
pkg/cdn/metadata/mongo/mongo.go
Outdated
| } | ||
| m.session = s | ||
|
|
||
| // TODO: collection as env var, or param to New()? together with user/mongo |
There was a problem hiding this comment.
I think this is ok as a constant for now. we can make it an env var if we need to, later on
pkg/cdn/metadata/rdbms/rdbms.go
Outdated
| // that satisfies the Getter and Setter interfaces. You must call | ||
| // Connect() on the returned store before using it. | ||
| // connectionName | ||
| func NewRDBMSStorage(connectionName string) *MetadataStore { |
There was a problem hiding this comment.
This name also "stutters" because it will be called with rdbms.NewRDBMSStorage - can you change this name to NewStorage instead?
Implements rdbms support and Mongo Saver for #147