Skip to content
This repository has been archived by the owner on Dec 10, 2022. It is now read-only.

Remove sample config, reads env var by default #5

Merged
merged 6 commits into from May 27, 2017

Conversation

Naramsim
Copy link
Collaborator

Please, look at #4 before, if you want to merge, merge after #4.
This PR contains your config.php removed from .gitignore, and deletes the sample one.

Inside config.php I assign variables looking from env and Dotenv.

One thing: define( 'SLACK_KEY__TXXXXXXXX', 'xoxp-0000000000-000000000000-000000000000-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx' ); You are defining a key "SLACK_KEY__TXXXXXXXX", why all those Xs?

@tdmalone
Copy link
Owner

Thanks, I will review! The TXXXXXXXX is the team ID, so that one install of Slackemon can be configured to run across multiple teams. Each team will have their own app token and OAuth key generated by Slack.

@juz501
Copy link
Collaborator

juz501 commented May 27, 2017

phpdotenv is not supposed to be used in a production environment according to the author vlucas vlucas/phpdotenv#63 (comment)

A proposed solution is to only load from .env in development only as below

Copying comment from #4

$dotenv = new Dotenv\Dotenv();
if(getenv('APP_ENV') === 'development') {
    $dotenv->load(__DIR__);
}

@tdmalone tdmalone self-assigned this May 27, 2017
@tdmalone tdmalone added this to the Quick deployment enhancements milestone May 27, 2017
@tdmalone tdmalone added this to In progress in Maintenance May 27, 2017
@tdmalone tdmalone merged commit 23a0071 into tdmalone:tim May 27, 2017
tdmalone added a commit that referenced this pull request May 27, 2017
@tdmalone
Copy link
Owner

Ah, just had a look at config.php. To reduce complexity at this stage, I'm going to not try to implement multiple teams at the moment. I would like to in future, but I need to focus on understanding the best way to structure the environment first - with one team.

@tdmalone tdmalone moved this from In progress to Done in Maintenance May 27, 2017
@tdmalone
Copy link
Owner

@Naramsim @juz501 This has been merged into tim and I've done further work on it too. I think I've got the config structure right now but value your further input. I need to test it further with Docker locally, and then ensure that the correct config vars are used through the app. Then I'll get Docker support into master.

@tdmalone
Copy link
Owner

Actually I can't build at the moment - Docker is giving me 'unknown instruction: USERMOD'. Will look into further tomorrow.

@juz501
Copy link
Collaborator

juz501 commented May 27, 2017

I don't think you can add the \ to end the line and then append some comments after that (in the Dockerfile). I'm just guessing though.

You can just get rid of the && \ and just put RUN per line in the Dockerfile and try.

@Naramsim
Copy link
Collaborator Author

@juz501, Hey, you were totally right, I´ve added the comments and didn´t test afterward.

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

Successfully merging this pull request may close these issues.

None yet

3 participants