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

Changes build flag to --lifecycle-docker-host #1113

Closed

Conversation

micahyoung
Copy link
Member

@micahyoung micahyoung commented Mar 15, 2021

  • Change build flag --docker-host to --lifecycle-docker-host
  • Adds explicit host-socket for default behavior value

Signed-off-by: Micah Young [email protected]

Summary

The pack build flag --docker-host introduced in #988 works as designed but user feedback (#1093) suggests the naming implies not only DOCKER_HOST for lifecycle is changed, but also DOCKER_HOST for pack (which could eventually be implemented, but is independent of the original purpose).

Here I change the name to --lifecycle-docker-host and cleanup some additional details:

  • Make the default value and behavior have explicit value: host-socket
  • Make --lifecycle-docker-host and --publish mutually exclusive and fail when used together (for publishing, lifecycle never has a dependency on daemon)

Out of scope for this, but potential next steps are:

  • Add --docker-host flag which would effectively set's pack's DOCKER_HOST
  • Make inherit be the default for --lifecycle-docker-host when pack's DOCKER_HOST is set

Output

Before

Usage output
--docker-host string          Address to docker daemon that will be exposed to the build container.

After

Usage output
--lifecycle-docker-host string   Address to docker daemon to expose for lifecycle in the build container.

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #1093

@micahyoung micahyoung changed the title Adds explicit docker-host opt default Draft: Adds explicit docker-host opt default Mar 15, 2021
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Mar 15, 2021
@github-actions github-actions bot added this to the 0.18.0 milestone Mar 15, 2021
@micahyoung micahyoung force-pushed the change-build-docker-host-opt branch 2 times, most recently from d17e3c1 to 11b0a1f Compare March 15, 2021 10:59
@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #1113 (d47dce9) into main (8759cb6) will decrease coverage by 0.53%.
The diff coverage is 96.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1113      +/-   ##
==========================================
- Coverage   80.52%   80.00%   -0.52%     
==========================================
  Files         136      136              
  Lines        8243     6004    -2239     
==========================================
- Hits         6637     4803    -1834     
+ Misses       1176      767     -409     
- Partials      430      434       +4     
Flag Coverage Δ
os_linux 80.00% <96.88%> (+0.01%) ⬆️
os_macos 77.49% <96.88%> (-0.02%) ⬇️
os_windows 100.00% <ø> (+19.57%) ⬆️
unit 80.00% <96.88%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@micahyoung micahyoung changed the title Draft: Adds explicit docker-host opt default Draft: Changes build opt to lifecycle-docker-host Mar 15, 2021
@micahyoung micahyoung force-pushed the change-build-docker-host-opt branch 2 times, most recently from 676b8a0 to d47dce9 Compare March 15, 2021 12:16
@micahyoung micahyoung changed the title Draft: Changes build opt to lifecycle-docker-host Changes build opt to lifecycle-docker-host Mar 15, 2021
@micahyoung micahyoung changed the title Changes build opt to lifecycle-docker-host Changes build flag to --lifecycle-docker-host Mar 15, 2021
@micahyoung micahyoung force-pushed the change-build-docker-host-opt branch from d47dce9 to 6d11343 Compare March 15, 2021 12:46
@micahyoung micahyoung marked this pull request as ready for review March 15, 2021 12:56
@micahyoung micahyoung requested a review from a team as a code owner March 15, 2021 12:56
Micah Young added 2 commits March 16, 2021 07:12
- Avoids nil-like empty default value

Signed-off-by: Micah Young <[email protected]>
- Previous name docker-host was confusing (Issue: 1093) and the name is best left for an eventual equivalent to `docker --host` https://docs.docker.com/engine/reference/commandline/cli/#:~:text=host%20value
- Validate mutual excluson of publish and lifecycle-docker-host

Signed-off-by: Micah Young <[email protected]>
@micahyoung micahyoung force-pushed the change-build-docker-host-opt branch from 6d11343 to bb9fe65 Compare March 16, 2021 11:16
@dwillist
Copy link
Contributor

Acceptance

(using a remote linux setup with docker exposed on port 2375)

Input

DOCKER_HOST=tcp://<-redacted->:2375 ./out/pack build test-build -p ~/workspace/VMware/paketo-buildpacks/samples/nodejs/yarn --lifecycle-docker-host=inherit

Output

Digest: sha256:e46e13f550df3b1fd694000e417d6bed534772716090f11a9876501ddeecb521
Status: Downloaded newer image for paketobuildpacks/builder:base
base-cnb: Pulling from paketobuildpacks/run
Digest: sha256:367a43536f60c21190cea5c06d040d01d29f4102840d6b3e1dcd72ed2eb71721
Status: Image is up to date for paketobuildpacks/run:base-cnb
===> DETECTING
5 of 8 buildpacks participating
paketo-buildpacks/ca-certificates 2.1.0
paketo-buildpacks/node-engine     0.2.2
paketo-buildpacks/yarn            0.1.2
paketo-buildpacks/yarn-install    0.2.4
paketo-buildpacks/yarn-start      0.0.4
===> ANALYZING
Previous image with name "test-build" not found
===> RESTORING
===> BUILDING

Paketo CA Certificates Buildpack 2.1.0
  https://github.com/paketo-buildpacks/ca-certificates
  Launch Helper: Contributing to layer
    Creating /layers/paketo-buildpacks_ca-certificates/helper/exec.d/ca-certificates-helper
Paketo Node Engine Buildpack 0.2.2
  Resolving Node Engine version
    Candidate version sources (in priority order):
                -> ""
      <unknown> -> ""

    Selected Node Engine version (using ): 14.16.0

  Executing build process
    Installing Node Engine 14.16.0
      Completed in 4.5s

  Configuring build environment
    NODE_ENV     -> "production"
    NODE_HOME    -> "/layers/paketo-buildpacks_node-engine/node"
    NODE_VERBOSE -> "false"

  Configuring launch environment
    NODE_ENV     -> "production"
    NODE_HOME    -> "/layers/paketo-buildpacks_node-engine/node"
    NODE_VERBOSE -> "false"

    Writing profile.d/0_memory_available.sh
      Calculates available memory based on container limits at launch time.
      Made available in the MEMORY_AVAILABLE environment variable.

Paketo Yarn Buildpack 0.1.2
  Executing build process
    Installing Yarn
      Completed in 510ms

Paketo Yarn Install Buildpack 0.2.4
  Resolving installation process
    Process inputs:
      yarn.lock -> Found

    Selected default build process: 'yarn install'

  Executing build process
    Running yarn install --ignore-engines --frozen-lockfile --modules-folder /layers/paketo-buildpacks_yarn-install/modules/node_modules
      Completed in 2.069s

  Configuring environment
    PATH -> "$PATH:/layers/paketo-buildpacks_yarn-install/modules/node_modules/.bin"

Paketo Yarn Start Buildpack 0.0.4
  Assigning launch processes
    web: node server.js

===> EXPORTING
Adding layer 'paketo-buildpacks/ca-certificates:helper'
Adding layer 'paketo-buildpacks/node-engine:node'
Adding layer 'paketo-buildpacks/yarn-install:modules'
Adding 1/1 app layer(s)
Adding layer 'launcher'
Adding layer 'config'
Adding layer 'process-types'
Adding label 'io.buildpacks.lifecycle.metadata'
Adding label 'io.buildpacks.build.metadata'
Adding label 'io.buildpacks.project.metadata'
Setting default process type 'web'
*** Images (3fd168b978e5):
      test-build
Adding cache layer 'paketo-buildpacks/node-engine:node'
Adding cache layer 'paketo-buildpacks/yarn:yarn'
Adding cache layer 'paketo-buildpacks/yarn-install:modules'
Successfully built image test-build

Result

👍 Looks perfect, really like the new interface.

Here is the docs issue associated with this PR

@micahyoung
Copy link
Member Author

After some Slack discussion, it feels like the --lifecycle-docker-host may leak the lifecycle concept more than needed and cause confusion. Some alternative names were suggested too:

  • --publish-docker-host
  • --export-docker-host

I'm going to close this for the moment and see if I can lay out a few scenarios that can help us ideate on some naming/syntax options... I'm feeling like we might still want to try and make --docker-host work, and be more configurable in the first place. And in the meantime, --docker-host as currently implemented will gather more user feedback.

@micahyoung micahyoung closed this Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new build flag --docker-host does not work as expected
2 participants