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

ref(data): rds implementation as DB interface #158

Closed
wants to merge 3 commits into from

Conversation

jackfrancis
Copy link
Member

wrapping the current RDS implementation inside a generic data.DB interface

contributes to #141

wrapping the current RDS implementation inside a generic data.DB interface

contributes to #141
@jackfrancis jackfrancis self-assigned this Jun 23, 2016
@jackfrancis jackfrancis added this to the v2.1 milestone Jun 24, 2016
// DB is an interface for managing a database
type DB interface {
Get() (*gorm.DB, error)
}
Copy link
Member

@arschles arschles Jun 24, 2016

Choose a reason for hiding this comment

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

I don't think we need this interface. We used to have it, but removed it because there was no need for a factory to create multiple *gorm.DBs - just passing one around worked just fine. Is there a reason for adding this that I'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just added a commit that should answer that question. The interface will make it easier for us to slot in alternate (to RDS) db implementations, and for testing/mocking as well.

The question is: is this an effective approach toward the objectives outlined in #141?

Copy link
Member

Choose a reason for hiding this comment

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

This abstraction will solve #141, but why not just pass around a *sql.DB? I removed this interface a while back because that abstraction already does everything we need

@smothiki smothiki added the LGTM1 label Jun 24, 2016
dBInstance = os.Getenv(dBInstanceKey)
dBUser = os.Getenv(dBUserKey)
dBPass = os.Getenv(dBPassKey)
)
Copy link
Member

Choose a reason for hiding this comment

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

it probably makes more sense to use envconfig or similar here. thoughts?

@jackfrancis jackfrancis modified the milestones: v2.2, v2.1 Jun 28, 2016
@arschles
Copy link
Member

moving out of 2.2, as #162 was also moved

@arschles arschles removed this from the v2.2 milestone Jul 18, 2016
@jackfrancis jackfrancis closed this Aug 1, 2016
@jackfrancis jackfrancis deleted the rds-generalize branch August 1, 2016 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants