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

feat: set builder images in templates and .faas.yaml #136

Merged
merged 1 commit into from Sep 24, 2020

Conversation

lance
Copy link
Member

@lance lance commented Sep 23, 2020

This commit adds a .builder.yaml file to each template directory. In the file
there is at the moment a single key/value pair, "default: <image>", where the
actual builder image name is <image>. Using a mapping allows the future
possibility that a user may specify a builder image by name via a flag on the
command line.

When a project is initialized, the .builder.yaml file is read, and the default
builder is saved in the project's .faas.yaml file. The .faas.yaml file is then
consulted when building an image with faas build. If the builder image is
specified, then the builder will use it. Otherwise, it will fallback to the
defaults. This allows developers to create custom builders, and specify them
in the configuration file.

An open question is whether the .builder.yaml file should be deleted after it
is read during project initialization. The init command is the only time the
file is consulted, and it may be confusing for the user if they discover a
second configuration-y file in the project directory.

Fixes: #120

@lance lance added kind/enhancement templates/node Related to the Node.js function templates templates/go Related to the Go function templates templates/quarkus Related to the Quarkus function templates source/configuration labels Sep 23, 2020
@lance lance requested a review from a team September 23, 2020 21:46
@lance lance self-assigned this Sep 23, 2020
@matejvasek
Copy link
Contributor

An open question is whether the .builder.yaml file should be deleted after it
is read during project initialization. The init command is the only time the
file is consulted, and it may be confusing for the user if they discover a
second configuration-y file in the project directory.

I think it's OK to delete the file for now.

In future I will add a feature for build modes (like release, debug), that means the .builder.yaml should be renamed to.builders.yaml since the file will contain multiple builders. Question is whether this map of builder will be copied to .faas.yaml if so then it can be deleted, otherwise it would need to be preserved. But that is question for future, I'll take care that.

@@ -0,0 +1 @@
default: nodejs quay.io/boson/faas-go-builder
Copy link
Contributor

Choose a reason for hiding this comment

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

nodejs ?

Copy link
Contributor

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, there's just the typo in the go events builder

+1 to delete the file afterwards

if f.Builder != "" {
packBuilder = f.Builder
} else {
packBuilder = Runtimes[f.Runtime]
Copy link
Contributor

Choose a reason for hiding this comment

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

Runtimes var name seems odd in this context, maybe it could be Buildpacks or Runtime2Buildpack, I know you haven't introduced that name.

@zroubalik
Copy link
Contributor

In future I will add a feature for build modes (like release, debug), that means the .builder.yaml should be renamed to.builders.yaml since the file will contain multiple builders. Question is whether this map of builder will be copied to .faas.yaml if so then it can be deleted, otherwise it would need to be preserved. But that is question for future, I'll take care that.

so what if we name the file .builders.yaml from the start, since we know that we will add more of them

This commit adds a .builder.yaml file to each template directory. In the file
there is at the moment a single key/value pair, "default: <image>", where the
actual builder image name is <image>. Using a mapping allows the future
possibility that a user may specify a builder image by name via a flag on the
command line. For example,

```console
faas build --builder native
```

When a project is initialized, the .builder.yaml file is read, and the default
builder is saved in the project's .faas.yaml file. The .faas.yaml file is then
consulted when building an image with `faas build`. If the builder image is
specified, then the builder will use it. Otherwise, it will fallback to the
defaults. This allows developers to create custom builders, and specify them
in the configuration file.

After extracting the builder image from .builder.yaml in the project directory,
this file is deleted.

This commit also adds Verbose to the init command.
@lance
Copy link
Member Author

lance commented Sep 24, 2020

I've incorporated all of the feedback @zroubalik @matejvasek - ty!

@lance lance merged commit d6e131f into knative:main Sep 24, 2020
@lance lance deleted the configurable-builders branch September 24, 2020 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement templates/go Related to the Go function templates templates/node Related to the Node.js function templates templates/quarkus Related to the Quarkus function templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not hard code builder image locations
3 participants