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

Setup database #20

Merged
merged 34 commits into from
Feb 18, 2021
Merged

Setup database #20

merged 34 commits into from
Feb 18, 2021

Conversation

gutakk
Copy link
Contributor

@gutakk gutakk commented Feb 15, 2021

What happened 👀

Resolve #4

  • Add database docker container for development and test environments.
  • Add env-setup and env-teardown make command to run and stop database container.
  • Add Viper as config management tools.
  • Add toml config file with dev and test database url.
  • Load config to get the database url vary by gin mode.
  • Add gorm as an ORM.
  • Application and test are connect to database.
  • Add common get config by type functions (bool, float, int and string) as a helper.
  • Add get database connection function as a helper.

Insight 📝

I choose Viper because Viper is one of the most popular config management tools that support TOML format as TOML widely use for config type in Go.

You can get value from toml config file using viper as following.

# toml config file

[debug]
database_url = "db"

[test]
database_url = "test db"

You can get value using viper like this

// debug env
viper.GetString("debug.database_url")

// test env
viper.GetString("test.database_url")

How about release environment?
Since this release is focusing only Heroku, all env on Heroku doesn't contain prefix and TOML file doesn't support environment injection so I need to create function GetConfigPrefix vary by gin.Mode.

You can get the config value using helpers function for configs that depend on gin.Mode() and can use viper directly for config that not depend on gin.Mode().

Proof Of Work 📹

  • Run development server (This will read database url from debug.database_url in config/app.toml)
    Cen7Tpk8s8

  • Run test (This will read database url from test.database_url in config/app.toml)
    N1MHgVDZit

  • Run application with release environment without provide DATABASE_URL
    qV7rLOMniD

  • Run application with release environment with DATABASE_URL
    9mE91KSq21

@gutakk gutakk added this to the 1.0.0 milestone Feb 15, 2021
@gutakk gutakk self-assigned this Feb 15, 2021
@gutakk gutakk force-pushed the feature/setup-database branch 2 times, most recently from a82da6e to 8bc695e Compare February 15, 2021 11:27
@gutakk gutakk marked this pull request as ready for review February 15, 2021 11:53
@gutakk gutakk linked an issue Feb 15, 2021 that may be closed by this pull request
{{cookiecutter.app_name}}/config/app.toml Show resolved Hide resolved
{{cookiecutter.app_name}}/bootstrap/database.go Outdated Show resolved Hide resolved
{{cookiecutter.app_name}}/docker-compose.dev.yml Outdated Show resolved Hide resolved
"github.com/spf13/viper"
)

func GetConfigPrefix() string {
Copy link
Member

Choose a reason for hiding this comment

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

Since this method is only used internally, how about not exporting it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I export this function because I want to write unit test. config_test's package is helpers_test.

Copy link
Member

Choose a reason for hiding this comment

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

I saw that but I am not sure of the value-added to have such test as it will be implicitly tested in the other public methods.

{{cookiecutter.app_name}}/config/app.toml Outdated Show resolved Hide resolved
"gorm.io/gorm"
)

// Get database connection from viper config
Copy link
Member

Choose a reason for hiding this comment

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

Why separating this from bootstrap/database.go? Couldn't this be together? I don't see the point of having a helper for that 🤔 Also note that I did not see where GetDBConnection is actually used. I would assume that InitDatabase would use it but it's not. So for now, it seems like an unused method.

Copy link
Contributor Author

@gutakk gutakk Feb 16, 2021

Choose a reason for hiding this comment

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

Gorm does not have any function to get database connection after initialize so I need to create a function to get database connection from viper config. I think bootstrap should contains only initializer stuffs. That's why I decide to put GetDBConnection function at helpers. But yes this function is unused for now, I will remove this in this PR and will put it again in the PR that need to use database connection.

Removed in 28bb423

Copy link
Member

Choose a reason for hiding this comment

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

Helpers use should be minimized. It should not be your go-to place for code 🙈 bootstrap should indeed contain all initialization code but that also include any secondary methods like such helpers too. Keeping the implementation code close to where the methods are declared is a best practice. Especially, in this case, the "helper" method does not need to be shared as it will be used only in bootstrap and nowhere else.

Base automatically changed from feature/setup-unit-test to develop February 17, 2021 10:51
@gutakk gutakk merged commit af44117 into develop Feb 18, 2021
@gutakk gutakk deleted the feature/setup-database branch February 18, 2021 10:25
@gutakk gutakk mentioned this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup database
4 participants