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

builder-build fix for DOKKU_DOCKERFILE_CACHE_BUILD and DOKKU_DOCKER_BUILD_OPTS variables #5232

Merged
merged 2 commits into from
Jul 17, 2022

Conversation

YuukiHogo
Copy link
Contributor

Documentation notices $DOKKU_DOCKERFILE_CACHE_BUILD and $DOKKU_DOCKER_BUILD_OPTS variables set by dokku config:set, but this variables does not fetched from app config in `builder-dockerfile.

@@ -36,7 +38,7 @@ trigger-builder-dockerfile-builder-build() {
declare -a ARG_ARRAY
eval "ARG_ARRAY=($DOCKER_ARGS)"

"$DOCKER_BIN" image build "${DOCKER_BUILD_LABEL_ARGS[@]}" $DOKKU_GLOBAL_BUILD_ARGS "${ARG_ARRAY[@]}" "${DOKKU_DOCKER_BUILD_OPTS[@]}" -t $IMAGE .
"$DOCKER_BIN" image build "${DOCKER_BUILD_LABEL_ARGS[@]}" $DOKKU_GLOBAL_BUILD_ARGS "${ARG_ARRAY[@]}" ${DOKKU_DOCKER_BUILD_OPTS} -t $IMAGE .
Copy link
Member

Choose a reason for hiding this comment

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

Why did you make this particular change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User defines DOKKU_DOCKER_BUILD_OPTS variable in dokku config:set command as string. Also builder-build#L32 modifies this variable as string. For consistency it is reasonable to use variable as string, not array.

Quoted variable leads to incorrect arguments for docker build image command. I.e.:
dokku config:set app DOKKU_DOCKER_BUILD_OPTS='--build-arg ENV=prod'
leads to
docker image build <...> '--build-arg ENV=prod' <...>

Also empty (default) value for DOKKU_DOCKER_BUILD_OPTS variable makes docker build image to emit error because of quotes.

Copy link
Member

Choose a reason for hiding this comment

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

Do you know how this might handle backticks in those strings? Basically, is it possible to set the following docker option and have it do the right thing? Mind testing?

dokku docker-options:add $APP deploy '--label traefik.http.routers.node-js-app.rule=Host(`node-js-app.dokku.arketyped.net`)'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested using proposing changes.
dokku config:set app DOKKU_DOCKER_BUILD_OPTS='--label traefik.http.routers.node-js-app.rule=Host(`node-js-app.dokku.arketyped.net`)'
Trace:

remote: + declare -a ARG_ARRAY
remote: + eval 'ARG_ARRAY=()'
remote: ++ ARG_ARRAY=()
remote: + docker image build --label=org.label-schema.schema-version=1.0 --label=org.label-schema.vendor=dokku --label=com.dokku.app-name=app --label=com.dokku.image-stage=build --label=org.label-schema.schema-version=1.0 --label=org.label-schema.vendor=dokku --label=dokku --label 'traefik.http.routers.node-js-app.rule=Host(`node-js-app.dokku.arketyped.net`)' -t dokku/app:latest .

Copy link
Member

Choose a reason for hiding this comment

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

Did the resulting container have that tag? Mind posting the output of the docker inspect on both the resulting image and deployed container??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Output of docker inspect dokku/app:

            "Labels": {
                "com.dokku.app-name": "app",
                "com.dokku.image-stage": "release",
                "dokku": "",
                "org.label-schema.schema-version": "1.0",
                "org.label-schema.vendor": "dokku",
                "traefik.http.routers.node-js-app.rule": "Host(`node-js-app.dokku.arketyped.net`)"
            }

Output of docker inspect 9c0d4cf0ea6d:

            "Labels": {
                "com.dokku.app-name": "app",
                "com.dokku.container-type": "deploy",
                "com.dokku.dyno": "worker.1",
                "com.dokku.image-stage": "release",
                "com.dokku.process-type": "worker",
                "dokku": "",
                "org.label-schema.schema-version": "1.0",
                "org.label-schema.vendor": "dokku",
                "traefik.http.routers.node-js-app.rule": "Host(`node-js-app.dokku.arketyped.net`)"
            }

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I'll merge once tests pass :)

@josegonzalez josegonzalez merged commit beec7f8 into dokku:master Jul 17, 2022
github-actions bot pushed a commit that referenced this pull request Jul 17, 2022
# History

## 0.27.8

Install/update via the bootstrap script:

```shell
wget https://raw.githubusercontent.com/dokku/dokku/v0.27.8/bootstrap.sh
sudo DOKKU_TAG=v0.27.8 bash bootstrap.sh
```

### Bug Fixes

- #5232: @YuukiHogo builder-build fix for DOKKU_DOCKERFILE_CACHE_BUILD and DOKKU_DOCKER_BUILD_OPTS variables

### New Features

- #5255: @josegonzalez export DOKKU_COMMAND for use in authentication systems

### Other

- #5250: @dependabot[bot] chore(deps): bump flask from 2.1.2 to 2.1.3 in /tests/apps/multi
- #5251: @dependabot[bot] chore(deps): bump flask from 2.1.2 to 2.1.3 in /tests/apps/python-flask
- #5245: @dependabot[bot] chore(deps-dev): bump heroku/heroku-buildpack-php from 221 to 222 in /tests/apps/php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants