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

Support Docker --build-arg option #161

Merged
merged 5 commits into from
Dec 9, 2021

Conversation

damiantw
Copy link
Contributor

@damiantw damiantw commented Dec 7, 2021

Our Vapor project that uses the Docker runtime depends on certain PHP extensions that have .ini configuration values that need to be set at image build time.

The value of these configurations vary by environment and also contains some sensitive values such as API keys.

To prevent hardcoding these values in the Dockerfile, we've defined sed commands in the build vapor.yml key of each environment that replace placeholder strings in our Dockerfiles prior to the image being built.

It's a kind of hacky approach. it'd be more ideal if we could make use of the Dockerfile ARG keyword and the docker build --build-arg option.

This PR allows for defining a build-arg key for each environment in vapor.yml.

ex:

id: 1
name: my-project
environments:
  staging:
    memory: 1024
    cli-memory: 512
    runtime: docker
    build-arg:
      FOO: BAR
      FIZZ: BUZZ
    build:
      - 'COMPOSER_MIRROR_PATH_REPOS=1 composer install'
      - 'php artisan event:cache'
      - 'npm ci && npm run dev && rm -rf node_modules'

Build arguments can also be provided to the deploy and build commands. This can be leveraged to inject secret values from the CI pipeline.

ex:

vapor deploy staging --build-arg FOO=BAR --build-arg FIZZ=BUZZ

vapor build staging --build-arg FOO=BAR --build-arg FIZZ=BUZZ

The merged build arguments from vapor.yml and the deploy/build command options will be passed to the docker build command.

If a build argument is present in both vapor.yml and as a CLI option, the CLI option value will take priority.

@nunomaduro nunomaduro self-requested a review December 8, 2021 16:59
@nunomaduro
Copy link
Member

@damiantw Interesting - there is any reason why we don't pass all environment variables, from the $_ENV, as build arguments instead of having to define them all in our vapor.yml file?

@nunomaduro nunomaduro added the question Further information is requested label Dec 8, 2021
@damiantw
Copy link
Contributor Author

damiantw commented Dec 8, 2021

@damiantw Interesting - there is any reason why we don't pass all environment variables, from the $_ENV, as build arguments instead of having to define them all in our vapor.yml file?

@nunomaduro I'm not opposed but I suppose there's the potential for unexpected conflicts with environment variables for people who upgrade vapor-cli and are unaware of this change (not sure this warrants a major version release).

For example, someone could have a Dockerfile like the following.

ARG VERSION=php81
FROM laravelphp/vapor:${VERSION}

If their CI environment has a VERSION environment variable that corresponds to the version of the app being built or something like that things will break.

Maybe that's an extreme edge case but it definitely feels safer to require explicitly specifying the build arguments in vapor.yml/as CLI arguments at build time.

Or maybe a vapor.yml configuration like envAsDockerBuildArgs: true to opt into that functionality?

@nunomaduro
Copy link
Member

Lets keep your initial proposal for now, I will test it tomorrow, and ship if it works as expected.

@nunomaduro nunomaduro merged commit 00f4c00 into laravel:master Dec 9, 2021
@nunomaduro
Copy link
Member

Looking good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants