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

Migrate the DB config from yaml template to viper #1321

Merged
merged 6 commits into from Nov 12, 2018

Conversation

2 participants
@chrisgilmerproj
Contributor

chrisgilmerproj commented Nov 8, 2018

Description

Pop currently uses a YAML config file that is a go template to load environment variables. This indirect approach should be changed to manually creating a connection on startup while using viper to capture the configuration values.

Reviewer Notes

I'm a bit worried that I'm overly verbose with error messages and would love input about it.

Setup

Start the server and the app. You should be able to get any db query to work.

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@pjdufour-truss

This comment has been minimized.

Contributor

pjdufour-truss commented Nov 8, 2018

Could you add more context to your zap errors? I think a connection can be output as a url string that can be added as zap.String("url", connstr). And then maybe manually try a bad set of connection details and see what happens.

@pjdufour-truss

This comment has been minimized.

Contributor

pjdufour-truss commented Nov 8, 2018

Or maybe just the username, host, port, and db separately, so we don't add password to logs by accident?

@chrisgilmerproj

This comment has been minimized.

Contributor

chrisgilmerproj commented Nov 8, 2018

Yeah, it should be hard to add more context there. Also, I noticed that all the other binaries in ./bin/ use the database.yml file. Should I remove those references and also use pflags/viper and my new initDatabase fn? I'm mostly worried that they will be forgotten and I'd love to keep everything consistent. The change is just a lot of grunt work and it might blow up the size of this PR.

@pjdufour-truss

This comment has been minimized.

Contributor

pjdufour-truss commented Nov 9, 2018

IMHO, it's fine to keep the other binaries as is for this PR, since this is a non-destructive change (if we keep the database.yml file there). We'd want to add something to the backlog to update all the binaries.

@chrisgilmerproj

This comment has been minimized.

Contributor

chrisgilmerproj commented Nov 9, 2018

This appears to have gone up to experimental just fine and I can make changes to the DB there.

I have one more change before I'd be ready to merge this.

@pjdufour-truss

This comment has been minimized.

Contributor

pjdufour-truss commented Nov 9, 2018

Could you make the db-port an Int config variable and then use strconv.Itoa to pass it to pop? We want to leverage the built-in validation with Viper as much as possible (before we start adding our own on top of it).

@chrisgilmerproj

This comment has been minimized.

Contributor

chrisgilmerproj commented Nov 9, 2018

I think I'll be merging this Monday just to make sure we have everyone around if there's an issue.

// Set up the connection
connection, err := pop.NewConnection(&dbConnectionDetails)
if err != nil {
logger.Error("Failed create DB conection", zap.Error(err))

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Nov 9, 2018

Contributor

Fix typo! Thanks @kilbergr !

@chrisgilmerproj chrisgilmerproj merged commit 21c3b0a into master Nov 12, 2018

12 checks passed

ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: deploy_experimental_app Your tests passed on CircleCI!
Details
ci/circleci: deploy_experimental_app_client_tls Your tests passed on CircleCI!
Details
ci/circleci: deploy_experimental_migrations Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details

@chrisgilmerproj chrisgilmerproj deleted the cg_161272729_migrate_db_config_to_viper branch Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment