openfaas / faas-cli

Official CLI for OpenFaaS
https://www.openfaas.com/
Other
798 stars 224 forks source link

[Feature] Provide --build-args for functions in stack.yml #687

Closed LucasRoesler closed 4 years ago

LucasRoesler commented 5 years ago

This was originally proposed in https://github.com/openfaas/faas-cli/issues/262 but was eventually implemented as flags on the faas-cli. This CLI behavior is documented here https://docs.openfaas.com/cli/build/#30-pass-custom-build-arguments

To ensure reproducible and portable builds, we should support supplying build args via the YAML stack file.

My actions before raising this issue

Expected Behaviour

It should be possible for both of these commands to produce the same build

faas-cli build
faas-cli build --build-arg ARGNAME1=argvalue1

Current Behaviour

The only way to set build args is via tha CLI

faas-cli build --build-arg ARGNAME1=argvalue1

Possible Solution

Allow specifying a map of build args per function in the stack yaml, for example

functions:
  url-ping:
    lang: python
    handler: ./sample/url-ping
    image: alexellis2/faas-urlping
    build_arg:
        cookies: "yup"
        pie: "please"
        brownies: "of_course"
alexellis commented 5 years ago

I think this might fit well under the build options section? Thoughts?

LucasRoesler commented 5 years ago

I think that would require a refactor and a breaking change, unfortunately. Ithink the original implementation assumes that build options == ADDITIONAL_PACKAGES, but I would have to double check

rgee0 commented 5 years ago

I thought we parsed them for the docker client to pass into the template dockerfile where the user has added the corresponding ARG. The ADDITIONAL_PACKAGES part is how the build_options are passed through & using it as a build-arg was a nice little bi-product of the implementation.

alexellis commented 5 years ago

I'm moving this to openfaas/faas-cli, hope people don't mind.. just let me know otherwise 🛩

alexellis commented 5 years ago

I would like to see if we can get it into build_options, even if we write a migrator like @viveksyngh started looking into for the OpenFaaS YAML config file.

Should this be a map or an array? I guess it's really a KV?

cconger commented 5 years ago

I'm a fan of distributing the build options down to the end function yaml as much as possible. It's ideal if you have as much of the function definition close to the function and out of the template, which is a big benefit of having the additional packages wiring. If we had build_args available in the function.yml file would there still be a benefit to having language level build flags (aside from backwards compatibility)?

alexellis commented 5 years ago

Good question. In a sense the packages are maintained i.e. dev and mysql are not 1:1 but map to curated packages.

alexellis commented 5 years ago

@rgee0

https://github.com/openfaas/faas-cli/issues/687#issuecomment-528030679

That's correct

We now have a use-case where we want to enable Go modules (some of the time) and @bmcstdio made a --build-arg available for that. How do we move this forward?

Today build_options looks like this:

build_options:
- ca-certificates

Taken from https://docs.openfaas.com/reference/yaml/#function-build-options

I don't know how heavily it's being used, but we could use a schema version change and bump it to be richer.

build_options:
  bundles:
   - ca-certs
  build_args:
    GO_MOD: 1
chrisrichard commented 4 years ago

If you can provide a bit of guidance on implementation we might be able to help with this one.

LucasRoesler commented 4 years ago

From how I understand the conversation, i think @alexellis wants to introduce the following yaml structure

version: 1

provider:
  name: openfaas
  gateway: http://127.0.0.1:8080

functions:
  url-ping:
    lang: python
    handler: ./sample/url-ping
    image: alexellis2/faas-urlping
    build_options:
      bundles:
      - ca-certs
      build_args:
        GO_MOD: 1

there are a couple important change in this structure

  1. a version value. This would allow us to introduce a breaking change in the function spec. We would need to freeze the current spec as version: 0 and when version is missing we assume 0. Ideally we should be just parsing to a single internal structure, just using a different parser/loader based on the version in the file. That way the internal code only needs to deal with a single structure.
  2. the build_options would change from a list to a map

    build_options:
      bundles:
      - ca-certs
      build_args:
        GO_MOD: 1

    this change would allow us to expand the build time options to now include the build_args map. This map would be translated to the docker --build-args flags during the docker build. This would need an update here https://github.com/openfaas/faas-cli/blob/0ab4012784510381e2b90c031ede3d77197b10b1/builder/build.go#L125-L135 and here https://github.com/openfaas/faas-cli/blob/0ab4012784510381e2b90c031ede3d77197b10b1/builder/build.go#L239-L276

    The CLI would need to be updated to read the build option packages from build_options.bundles. See here in the source code https://github.com/openfaas/faas-cli/blob/0ab4012784510381e2b90c031ede3d77197b10b1/builder/build.go#L52

  3. of course the docs would need to be updated as well https://docs.openfaas.com/reference/yaml/#function-build-options we would need to decide how we want to document the version behavior. I am not sure if we want to follow the example of postgres and have entire copies of the documentation for each release or just add a branch in that part of the documentation for the two different versions.

If we want to avoid a breaking change, then we need a new name for the build_options. Perhaps we can deprecate build_options over the next couple of months and replace it with build_config field? Use the same structure described as above but then we need to worry less about the version field.

alexellis commented 4 years ago

I'm not seeing much usage in OSS repos: (if this search is working as expected) https://github.com/search?q=faas+build_options&type=Code

alexellis commented 4 years ago

I'm leaning towards build_config, we could also just extend the existing field:

From i.e.

build_options:
- ca-certificates
- build-essential

to:

build_options:
- ca-certificates
- "--build-arg GO111MODULE=on"
LucasRoesler commented 4 years ago

I think i would rather introduce a breaking change or use build_config versus mixing the build options and flag parsing

martindekov commented 4 years ago

So should we stay with:

build_options:
- ca-certificates
- build-essential

And not mix config and flag parsing? Did you reach consensus?

abhayvatoo commented 4 years ago

From the above conversation, I think @alexellis and @LucasRoesler are good with deprecating build_option and enabling new build_config option. The new yaml structure will look like:

 version: 1

provider:
  name: openfaas
  gateway: http://127.0.0.1:8080

functions:
  url-ping:
    lang: python
    handler: ./sample/url-ping
    image: alexellis2/faas-urlping
    build_config: # build_options changed to build_config
      build_args: # would be translated to the docker --build-args flags during the docker build
        GO_MOD: 1

@chrisrichard and I am working on this. I believe we understood the issue correctly. Please let us know otherwise or in case of any issue.

EDITED: after discussion with @chrisrichard