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

feat: add db package #38

Merged
merged 8 commits into from
Aug 30, 2022
Merged

feat: add db package #38

merged 8 commits into from
Aug 30, 2022

Conversation

ravisuhag
Copy link
Member

@ravisuhag ravisuhag commented Aug 20, 2022

Common package for connecting to SQL databases

@ravisuhag ravisuhag linked an issue Aug 20, 2022 that may be closed by this pull request
@ravisuhag ravisuhag requested a review from krtkvrm August 20, 2022 22:17
sqlx/sqlx.go Outdated Show resolved Hide resolved
Copy link

@StewartJingga StewartJingga left a comment

Choose a reason for hiding this comment

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

Since this is a pretty important part of an application (interacting with the main source) and we will use it across our odpf services, should we consider adding test? at least for migration and transaction.

wdyt?

sqlx/sqlx.go Outdated Show resolved Hide resolved
sqlx/sqlx.go Outdated Show resolved Hide resolved
sqlx/migrate.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
db/migrate.go Outdated Show resolved Hide resolved
db/migrate.go Outdated Show resolved Hide resolved
db/config.go Outdated
)

type Config struct {
Driver string `yaml:"driver" mapstructure:"driver"`
Copy link
Member

Choose a reason for hiding this comment

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

do we need these yaml & mapstructure tags here?

Copy link
Member Author

@ravisuhag ravisuhag Aug 26, 2022

Choose a reason for hiding this comment

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

So that when we compose this in a higher level config struct, we can tell viper what names to use in the configs passed through yaml file. Any other suggestion to achieve that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this the best way to do it.

P.S.: I think, these kinds of tags are okay (even good) to be there for any struct even if you don't have immediate need -- They don't really have an inherent cost and when you actually need to use the struct to show/read yaml/json etc. it just works. (It just says "if you were to represent this in YAML, these are the key names you should be using")

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

db/migrate.go Outdated Show resolved Hide resolved
@ravisuhag
Copy link
Member Author

Since this is a pretty important part of an application (interacting with the main source) and we will use it across our odpf services, should we consider adding test? at least for migration and transaction.

wdyt?

@StewartJingga @krtkvrm Added high-level tests for both. Please check. Let me know if we need to make them more strict.

db/db.go Outdated Show resolved Hide resolved
@ravisuhag ravisuhag changed the title feat: add sqlx package feat: add db package Aug 30, 2022
@ravisuhag ravisuhag merged commit afb9357 into main Aug 30, 2022
@ravisuhag ravisuhag deleted the sqlx branch August 30, 2022 22:46
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.

Add sql database common package
7 participants