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.

Sign up for GitHub

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

Jump to bottom

Throw descriptive error message for absent .env #1546

Merged
merged 6 commits into from
Oct 30, 2020
Merged

Throw descriptive error message for absent .env #1546

merged 6 commits into from
Oct 30, 2020

Conversation

kael89
Copy link
Collaborator

@kael89 kael89 commented Oct 28, 2020

Issue #:

[No issue]

@kael89 kael89 requested a review from edmofro October 28, 2020 05:46
@kael89
Copy link
Collaborator Author

kael89 commented Oct 28, 2020

@edmofro I assigned this to you directly because we were discussing about it the other day, and also I have a question for you related to this

kael89
kael89 commented Oct 28, 2020
View reviewed changes
packages/database/src/getConnectionConfig.js Outdated
user: process.env.DB_USER,
password: process.env.DB_PASSWORD,
database: process.env.DB_NAME,
ssl: ['true', undefined].includes(process.env.DB_DISABLE_SSL)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So here is a change: we currently set ssl to null only if DB_DISABLE_SSL is explicitly set to true. It seems to me that it should be the other way around: SSL should be disabled by default, and people can turn it on. But let me know if my understanding is wrong.

If this is correct, we may want to rename this to DB_ENABLE_SSL to make this "opt-in" behaviour clearer.

Also, I couldn't find a .env file in Last Pass where this was set to a value other than true, so I'm wondering whether it is used at all?

Copy link
Member

Choose a reason for hiding this comment

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

It's set to false if you are developing locally but want to connect to a remote database. In future, we aim to move the database across to a service like RDS, so we'd also want to use an ssl connection then.

I'd agree with your suggestion of the rename and default

kael89
kael89 commented Oct 28, 2020
View reviewed changes
packages/auth/.env.example
@@ -1,4 +1,4 @@
DB_DISABLE_SSL=
DB_ENABLE_SSL=
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if we should include this optional field in the examples or just skip it, I chose to include it

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it's good to include for completeness. Please add a note that the release process needs to include some manual changes to environment variables (the process is somewhat arduous)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add it (by the way I am the release manager this week). Could you explain the process to me in person (or point me to related documentation)?

Copy link
Member

Choose a reason for hiding this comment

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

For sure. This is the best documentation so far, but I will take you through it in person and we can add a doc just for that process

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.beyondessential.com.au/books/dev-ops/page/tupaia-main-server-structure-overview#bkmrk-environment-variable

edmofro
edmofro approved these changes Oct 28, 2020
View reviewed changes
Copy link
Member

@edmofro edmofro left a comment

Choose a reason for hiding this comment

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

Very nice breaking out two consts, that's much more clear.

Pre-approved with one suggested change

packages/database/src/getConnectionConfig.js Outdated
Comment on lines 29 to 32
if (config.ssl) {
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (config.ssl) {
return;
}

If ssl is set, it doesn't guarantee we have the other variables set up correctly (and we still need to)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure about that and actually I should have left a question-comment! Thanks for catching this!

kael89
kael89 commented Oct 29, 2020
View reviewed changes
packages/database/src/getConnectionConfig.js
},
};
validateEnv();
// Note: Must use functions to guarantee environment variables have loaded
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed this is the case... In previous commits, I removed the comment (and used const) without a problem, but I was probably lucky

This functionality now fails if functions are not used to retrieve the .env variables

@kael89 kael89 requested a review from edmofro October 29, 2020 20:57
@kael89
Copy link
Collaborator Author

kael89 commented Oct 30, 2020 •
edited
Loading

@edmofro I'm going to merge this into dev, and follow the new .env update instructions to remove DB_SSL_DISABLE (not required anymore) for the packages whose .env.example files were updated in this PR

@edmofro
Copy link
Member

edmofro commented Oct 30, 2020

Sounds good!

@kael89 kael89 mentioned this pull request Oct 30, 2020
Merged
3 tasks
@kael89 kael89 merged commit 84fe054 into dev Oct 30, 2020
@kael89 kael89 deleted the descriptive-error-message-for-absent-env branch October 30, 2020 01:35
@kael89
Copy link
Collaborator Author

kael89 commented Nov 4, 2020

@edmofro as discussed, it seems that in our current prod/dev instances (and LastPass), the following applies:

# meditrak-server
DB_DISABLE_SSL="false"
# web-config-server
DB_DISABLE_SSL="true"

Which implies that the presence of DB_DISABLE_SSL is significant in some cases. However, we have now removed support for this variable, but everything is working fine in dev. It may be that at some point (early stages of Tupaia development?) devs used SSL to connect to a database, and hence the need for this variable.

Given that we want to be able to connect to a remote DB, we can still retain support for DB_ENABLE_SSL, but remove it from all existing .env files since it is not currently required. The other option would be to completely remove it, and reintroduce it when required.

@kael89 kael89 mentioned this pull request Nov 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmofro edmofro edmofro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kael89 @edmofro