Allow extra options to be passed to docker build

[adamblake]

This is a first pass at implementing #141 (Ability to include extra options to docker build). It allows users to specify extra build options in chartpress.yaml on a per-image basis. This is useful for taking advantage of other build options that chartpress does not explicitly support.

I am not very familiar with pytest, so there are no tests included here. I can confirm that it is working using the --ssh option to clone a private GitHub repo in my local builds.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[manics]

A disadvantage of using a dict is that repeated arguments aren't allowed.

[consideRatio]

I support adding this feature!

Good point @manics! Go for a list, and the name should be more explicit i think. Args is too vague as it there are both docker build args and docker push args, and even helm cli args of possible relevance.

Perhaps dockerBuildArgs or maybe with an extra prefix so it is extraDockerBuildArgs.

[adamblake]

@consideRatio I chose extraOptions to intentionally separate the idea of "build arguments" (i.e. --build-arg=) and "docker build options" (i.e. arbitrary options). Calling it extraBuildArgs would make it easy to confuse with the other chartpressl.yaml key buildArgs which references the build arguments and not the options.

Since the docker build documentation calls these "options" I thought it was a good choice (here's the command interface from their docs docker build [OPTIONS] PATH | URL | -). I also didn't think that "docker" needed to be added to the key because it's nested in the images key and the other related keys don't have it (e.g. buildArgs).

Here's a more full example of a chartpress.yaml that would use this key. I made the extraOptions a list like @manics suggested. I also considered that some options have default values so the interface should allow you to pass no value to them.

charts:
  - name: some-jupyter-hub
    imagePrefix: a-nice-prefix/
    images:
      k8s-hub:
        valuesPath: hub.image
        buildArgs:
          TIMEOUT: "15m"
          COMMIT_REF: "1.4.5"
        extraOptions:
          - rm  # uses default value
          - label: "maintainer=octocat"
          - label: "version=0.1.0"
          - ssh: "default=secrets/secret-auth-key"    

I'll implement the list, and please let me know what you think would be the best key name!

[consideRatio]

Great point about the --build-args flag @adamblake! Hmmm, I'd love for it to be quite self-explanatory, but given that we also have --build-args causing some confusion I figure we would need to have it be extraDockerBuildOptions.

@manics, what do you think about the naming? I'm generally in favor of any name, almost no matter how long it is, that makes it quite clear directly from the name what it does.

I think I'd prefer an API that minimizes complexity and embedded logic, so for example...

charts:
  - name: some-jupyter-hub
    imagePrefix: a-nice-prefix/
    images:
      k8s-hub:
        valuesPath: hub.image
        buildArgs:
          TIMEOUT: "15m"
          COMMIT_REF: "1.4.5"
        extraBuildCommandArgs:
          - --rm
          - --label=maintainer=octocat
          - --label=version=0.1.0
          - --ssh=default=secrets/secret-auth-key
# Equivalently, one can split apart a args key/value into
# two separate elements in the list.
#      extraBuildCommandArgs:
#        - --rm
#        - --label
#        - maintainer=octocat
#        - --label
#        - version=0.1.0
#        - --ssh
#        - default=secrets/secret-auth-key

We currently have buildArgs support a structure like above though, where it's key/values are the value of the --build-arg flag... hmmm....

Brainstorming:

• extraOptions | extraArgs
• extraBuildOptions | extraBuildArgs
• extraDockerBuildOptions | extraDockerBuildArgs
• extraBuildCommandOptions | extraBuildCommandArgs
• extraDockerBuildCommandOptions | extraDockerBuildCommandArgs

Perhaps @manics @minrk could join in and opine on this?

1. We need a config name
2. We need to choose to accept:
   • strings only
   • dictionaries with a single key (null value -> empty string value)
   • strings or a dictionary with a single non-null key

I'm leaning towards extraDockerBuildCommandArgs and supporting strings only, where I'm quite clearly opined towards strings only but less clearly opined about the config name.

[manics]

I think extraDockerBuildCommandArgs / extraDockerBuildsArgs / extraDockerArgs / something along those lines is fine, I'm happy to trust @consideRatio's judgement on the final choice.

I think the mix of single values and dicts adds unnecessary complexity. These arguments are being passed directly to a command line program which expects a list of strings, so let's keep it as that?

[minrk]

I do think a simple opaque list of strings to pass through is the way to go.

One more option is to specify dockerBuildCommand itself the full CLI list (['docker', 'buildx', 'build', '--whatever-other-flags']). Then we don't have to decide whether to call them Args or Options or Flags :). This would allow things like specifying the abspath to docker, etc. if that matters.

For the extra options path, to avoid confusion with buildArgs which I think is unambiguous and has the right name, I'd avoid Args but the rest seem fine to me. I'd lean toward extraBuildCommand or extraBuildCommandOptions if you want my vote.

[adamblake]

It seems like a list of strings and a name like extraBuildCommandOptions is the consensus. I'll implement that and add some tests.

(I'll leave the other suggestion about the full command for now because I'm not sure how that would play with the other chartpress keys.)

[consideRatio]

It seems like a list of strings and a name like extraBuildCommandOptions is the consensus. I'll implement that and add some tests.

Sounds great to me, thank you @adamblake!

I think the name extraBuildCommandOptions is okay as it manages to a) avoid the confusion about --build-args, b) convey its about the docker build part, c) convey its in addition to other flags.

[adamblake]

I would fix the issue with the pre_commit GitHub Action, but I don't understand the error. I did run pre-commit run -a before committing though, and there were no errors.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[consideRatio]

Thank you @adamblake for the PR and @manics and @minrk for review!! 🎉 ❤️