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

edits for tutorial draft #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

edits for tutorial draft #1

wants to merge 1 commit into from

Conversation

katjuell
Copy link
Owner

No description provided.

Copy link

@hjet hjet left a comment

Choose a reason for hiding this comment

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

Some suggestions

- MONGO_PASSWORD=$MONGO_PASSWORD
- MONGO_HOSTNAME=$MONGO_HOSTNAME
- MONGO_PORT=$MONGO_PORT
- MONGO_DB=$MONGO_DB
Copy link

Choose a reason for hiding this comment

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

Would set the MONGO_HOSTNAME variable to db explicitly here, instead of setting it to db in the config parameter file. Would either leave it blank/unset or omit it from that file entirely. db is a "Compose-level" variable, so referring to it at the Compose level makes more sense and is cleaner, IMO. See comment on Tutorial markdown.

container_name: db
restart: unless-stopped
environment:
- MONGO_INITDB_ROOT_USERNAME=user
Copy link

Choose a reason for hiding this comment

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

Would avoid checking these (even more) sensitive parameters into version control. Would create a separate .env file (or use the same file) and externalize these from the Dockerfile.

@@ -0,0 +1,79 @@
#!/bin/sh

Copy link

Choose a reason for hiding this comment

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

Would maybe add a comment in the script linking out to the original on GH.

@@ -5,3 +5,5 @@ Dockerfile
.git
README.md
.gitignore
.env
db-init.js
Copy link

Choose a reason for hiding this comment

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

Would check db-init.js into version control (since it is code that gets executed), and modify the script so it pulls config/sensitive info from the already configured .env file. All config should be centralized and removed from code (and all code that is run should be checked in to VC).

@@ -76,4 +76,5 @@ typings/
.fusebox/

#DynamoDB Local files
.dynamodb/
.dynamodb/
db-init.js
Copy link

Choose a reason for hiding this comment

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

See above comment.

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.

2 participants