这是indexloc提供的服务，不要输入任何密码
Skip to content

builder-build fix for DOKKU_DOCKERFILE_CACHE_BUILD and DOKKU_DOCKER_BUILD_OPTS variables #5232

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

Merged
merged 2 commits into from
Jul 17, 2022
Merged

builder-build fix for DOKKU_DOCKERFILE_CACHE_BUILD and DOKKU_DOCKER_BUILD_OPTS variables #5232

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab45435
Update builder-build
YuukiHogo Jun 28, 2022
5182638
Merge branch 'master' into builder-dockerfile-patch-1
josegonzalez Jul 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion plugins/builder-dockerfile/builder-build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ trigger-builder-dockerfile-builder-build() {
dokku_log_info1 "Building $APP from Dockerfile"

local IMAGE=$(get_app_image_name "$APP")
local DOKKU_DOCKERFILE_CACHE_BUILD=$(config_get "$APP" "DOKKU_DOCKERFILE_CACHE_BUILD")
local DOKKU_DOCKER_BUILD_OPTS=$(config_get "$APP" "DOKKU_DOCKER_BUILD_OPTS")
local DOCKER_BUILD_LABEL_ARGS=("--label=dokku" "--label=org.label-schema.schema-version=1.0" "--label=org.label-schema.vendor=dokku" "--label=com.dokku.image-stage=build" "--label=com.dokku.builder-type=dockerfile" "--label=com.dokku.app-name=$APP")

pushd "$SOURCECODE_WORK_DIR" &>/dev/null
Expand All @@ -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

@josegonzalez josegonzalez Jul 5, 2022

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

@YuukiHogo YuukiHogo Jul 11, 2022

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

@josegonzalez josegonzalez Jul 13, 2022

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

@YuukiHogo YuukiHogo Jul 13, 2022

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

@josegonzalez josegonzalez Jul 13, 2022 •
edited
Loading

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

@YuukiHogo YuukiHogo Jul 13, 2022

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

@josegonzalez josegonzalez Jul 13, 2022

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 :)


plugn trigger post-build-dockerfile "$APP"
}
Expand Down