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

Instantiate logging depending on environment #1343

Merged
merged 8 commits into from Oct 10, 2018
Merged

Conversation

gbirke
Copy link
Member

@gbirke gbirke commented Sep 30, 2018

@gbirke gbirke force-pushed the configurable-logging branch 3 times, most recently from eb645bb to 2efb4a2 Compare October 1, 2018 16:51
@gbirke gbirke changed the title [WIP] Instantiate logging depending on environment Instantiate logging depending on environment Oct 1, 2018
@gbirke
Copy link
Member Author

gbirke commented Oct 1, 2018

@timEulitz @tzhelyazkova I'm unsure about some parts of the refactoring:

  • Is is clear enough how to set APP_ENV especially when running console commands, or should we add documentation?
  • Is injecting services after-the-fact into FunFunFactory ok or should we rather have a EnvironmentFactory interface and instantiate FunFunFactory with that, using it internally like with the ChoiceFactory? Existing FunFunFactory interface could be left as-is and environment-dependent instances could be passed on to EnvironmentFactory.

@gbirke gbirke changed the title Instantiate logging depending on environment [DNM] Instantiate logging depending on environment Oct 1, 2018
use WMDE\Fundraising\Frontend\Factories\FunFunFactory;
use WMDE\Fundraising\Frontend\Tests\Fixtures\FakeUrlGenerator;

class TestEnvironmentSetup implements EnvironmentSetup {
Copy link
Contributor

Choose a reason for hiding this comment

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

double space omg what does it mean?

@timEulitz
Copy link
Contributor

Is is clear enough how to set APP_ENV especially when running console commands, or should we add documentation?

It's clear to me but I don't think it would hurt to leave a short sentence about that (also with regards to the fact that we will be getting an intern soonish and that there will be other people in the future who might not know).

Is injecting services after-the-fact into FunFunFactory ok or should we rather have a EnvironmentFactory interface and instantiate FunFunFactory with that, using it internally like with the ChoiceFactory?

I think overall the EnvironmentFactory you propose would make the code better to read because you can more easily point at a named place in the code in which environment configurations are applied rather than a couple of random lines in the index files. You can also probably get rid of hard-coding the dev string value in the two entry files and move it to that class.

The thing that bugs me the most about it is that a Factory builds another Factory which seems very weird and kinda convoluted to me, but overall I think in this instance it's actually okay.

@gbirke gbirke force-pushed the configurable-logging branch 4 times, most recently from c18c189 to 3948185 Compare October 5, 2018 13:13
@gbirke gbirke changed the title [DNM] Instantiate logging depending on environment Instantiate logging depending on environment Oct 8, 2018
@gbirke gbirke force-pushed the configurable-logging branch 3 times, most recently from bb13d9a to d5f80ba Compare October 8, 2018 11:48
gbirke and others added 8 commits October 8, 2018 19:10
Change all app entry points to use APP_ENV environment variable to
determine environment name.
Use 'dev' as default.
Use "prod" when deploying.
Adapt Makefile
Explain environments in README.

Inspired by
https://symfony.com/doc/current/configuration/environments.html
Setup up entities that are environment-dependent in environment-specific
factories.

Change dev environment logging to standard error output and change
format to text instead of JSON.
Add `logging` configuration key.
Add factory for setting up logging depending on configuration key -
use error log, file or graylog.
Pass configuration to environment factories.
Added GELF library as a dependency for logging to Graylog.

Simplified production logging - removed the "FingersCrossed" handler and
the two different log outputs.
Also document how the campaign configurations are set up.
Make EnvironmentBootstrapper an instantiable class with overrideable
defaults, override them in the test environment.
Check that only the expected things are modified, but not how they're
modified.

Also simplify logger construction because each logger has only one,
predefined handler.
@timEulitz timEulitz merged commit 18bf39c into master Oct 10, 2018
@timEulitz timEulitz deleted the configurable-logging branch October 10, 2018 15:24
@gbirke gbirke mentioned this pull request Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants