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

Add default EnvVarProcessor for config.json #29

Merged
merged 8 commits into from
Sep 10, 2020
Merged

Add default EnvVarProcessor for config.json #29

merged 8 commits into from
Sep 10, 2020

Conversation

michaljurecko
Copy link
Contributor

@michaljurecko michaljurecko commented Sep 9, 2020 •
edited
Loading

Solving: https://keboola.atlassian.net/browse/COM-203

Changes:

  • In config.json placeholders are replaced by env variables.
  • Removed composer.lock - not needed for the library.

@michaljurecko michaljurecko force-pushed the webrouse-COM-203-env-processor branch from 54bee43 to aec1ddd Compare September 10, 2020 06:36
@michaljurecko michaljurecko force-pushed the webrouse-COM-203-env-processor branch from de08310 to d288781 Compare September 10, 2020 08:34
@michaljurecko michaljurecko marked this pull request as ready for review September 10, 2020 08:40
@michaljurecko
Copy link
Contributor Author

@tomasfejfar ahoj, mohol by si prosim spravit review? OJ ma 2 tyzdne dovolenku.

Ako som pisal v predhcadzajucom PR, tak EnvVarProcessor je sucastou symfony/dependency-injection a preto som ho nepouzil: #28 (comment)

Trocha som sa tym inspiroval, a da sa specifikovat datovy typ.
Ak by niekto potreboval, tak sa tam bude dala lahko pridat dalsia funkcionalita.

tomasfejfar
tomasfejfar requested changes Sep 10, 2020
View reviewed changes
Copy link
Contributor

@tomasfejfar tomasfejfar left a comment

Choose a reason for hiding this comment

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

Ne všechno je must have.

README.md Outdated Show resolved Hide resolved
src/AbstractDatadirTestCase.php Outdated Show resolved Hide resolved
src/EnvVarProcessor.php Show resolved Hide resolved
src/EnvVarProcessor.php Show resolved Hide resolved
src/EnvVarProcessor.php Show resolved Hide resolved
src/EnvVarProcessor.php Outdated Show resolved Hide resolved
src/EnvVarProcessor.php Show resolved Hide resolved
tests/phpunit/EnvVarProcessorTest.php Outdated Show resolved Hide resolved
@michaljurecko
Copy link
Contributor Author

Pridal som este metodu AbstractDatadirTestCase::getEnv, moze sa to v niektorych pripadoch hodit, kedze getenv vracia aj false, a musi sa to v kode osetrovat.

@michaljurecko
Copy link
Contributor Author

@tomasfejfar vdaka za review, fixol som to podla tvojich komentarov, ... 2 som nechal este otvorene, vid hore.

tomasfejfar
tomasfejfar approved these changes Sep 10, 2020
View reviewed changes
Copy link
Contributor

@tomasfejfar tomasfejfar left a comment

Choose a reason for hiding this comment

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

LGTM

@michaljurecko michaljurecko merged commit 9b2bf6d into master Sep 10, 2020
@michaljurecko michaljurecko deleted the webrouse-COM-203-env-processor branch September 10, 2020 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomasfejfar tomasfejfar tomasfejfar 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
@michaljurecko @tomasfejfar