Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

Rename config for docker #176

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Rename config for docker #176

wants to merge 4 commits into from

Conversation

ionphractal
Copy link

After changing the domains to xip.io, a local Docker development environment was unable to load images (failing with 401 unauthorized). That is because the JWT token audience uri was still set to localhost and hence the token was not recognized as valid.

The configuration package loads the config from NODE_ENV. And as there was no development config and Docker uses "development" I though I'd rename the config and extend it with the required parameters.

Does this break anything on your side? @appinteractive @roschaefer

@appinteractive
Copy link
Member

This would break the local environment without docker. You might want to use development-docker.json as a filename instead.

@ionphractal ionphractal changed the title Rename config for docker [WIP ]Rename config for docker Oct 21, 2018
@ionphractal ionphractal changed the title [WIP ]Rename config for docker [WIP] Rename config for docker Oct 21, 2018
@ionphractal
Copy link
Author

Hm so we might want to treat NODE_ENV=development-docker as development in code, right?

@roschaefer
Copy link
Contributor

@ionphractal great debugging! Could you use config/default-docker.json instead, please? We set NODE_APP_INSTANCE=docker in the Dockerfile. See the node-config wiki to learn how config files are loaded.

Apart from that 👍

"audience": "http://webapp.127.0.0.1.xip.io:3030"
}
},
"seeder": {
Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe remove seeder part completely. It should be up to the user if he or she wants to seed the database in docker.

Copy link
Author

Choose a reason for hiding this comment

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

changed what you suggested

@ionphractal ionphractal changed the title [WIP] Rename config for docker Rename config for docker Nov 23, 2018
@@ -4,6 +4,7 @@ services:
api:
environment:
- NODE_ENV=development
- NODE_APP_INSTANCE=docker
Copy link
Contributor

Choose a reason for hiding this comment

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

Not strictly required as it is set in the Dockerfile @ionphractal

While it is important to have it set in the docker container, I would not add it additionally in `docker-compose.yml` because other developer might believe, it is necessary to have it there.
@roschaefer
Copy link
Contributor

@appinteractive I restarted the build once, can you tell me what's wrong with codacy right now?

@appinteractive
Copy link
Member

@roschaefer codacy?

@roschaefer
Copy link
Contributor

It's failing the build.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants