Skip to content

Conversation

Smolevich
Copy link
Contributor

@Smolevich Smolevich commented Jun 26, 2018

On my opinion we don't need execute sed command in .travis.yml because Docker engine has arguments in Dockerfile and we can use it

  • Add arg PHP_VERSION in Dockerfile
  • Set arg PHP_VERSION in .travis.yml from env variable TRAVIS_PHP_VERSION
  • Package zlib1g-dev replaced from pecl install to apt-get install

@Smolevich
Copy link
Contributor Author

@jenssegers, can you review, please?

- .:/code
working_dir: /code
environment:
PHP_VERSION: ${PHP_VERSION}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a fixed value I guess, or at least provide a default value. Could you change this?

Great job on these changes btw!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i can set default value, but env variable PHP_VERSION set in travis script build

Copy link
Contributor Author

@Smolevich Smolevich Jun 27, 2018

Choose a reason for hiding this comment

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

Also in Docker file i popose operation installing composer to replace on this construction

FROM composer as composer-build #official docker image with composer (https://github.com/composer/docker/blob/master/1.6/Dockerfile)

COPY --from=composer-build /usr/bin/composer /usr/local/bin/composer

@Smolevich
Copy link
Contributor Author

@jenssegers, can you review changes?

@jenssegers jenssegers merged commit d2fe7da into mongodb:master Jul 22, 2018
@jenssegers
Copy link
Contributor

Great work!

mnphpexpert added a commit to mnphpexpert/laravel-mongodb that referenced this pull request Sep 2, 2024
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