Add new build driver for buildkit #1567

carolynvs · 2021-05-08T23:59:04Z

What does this change

Add --experimental flag to root porter command
Add experimental flags to config file. These features are disabled by default.
Add experimental flag for "build-drivers". This allows specifying the build driver, e.g. docker, buildkit, in the config file.
Add driver for buildkit.
Document drivers and how to configure them.
Document experimental flags
porter create generates a Dockerfile that uses buildkit when that feature is enabled

❓ As a follow-on should a bundle be able to say "I use buildkit!" and then fail if it's not enabled, or automatically pick that driver

example configuration to enable the feature:

~/.porter/config.toml

experimental = ["build-drivers"]
build-driver = "buildkit"

You can also set it with environment variables

export PORTER_EXPERIMENTAL=build-drivers (comma seperated)
export PORTER_BUILD_DRIVER=buildkit

or with flags

porter build --experimental build-drivers --driver buildkit

What issue does it fix

Closes #1268

Notes for the reviewer

As I was working on this I realized that we have some problems around when the configuration is loaded, and accessing it without accidentally getting a nil pointer exception. So I did a bit of refactoring to address since it was blocking the --experimental flag. It was prohibitive to refactor in a separate PR so hopefully having this smooshed together is reviewable.

Refactor porter's config and data structures so that we can't get nil pointer exceptions or try to access the configuration data before it is loaded.
Move helper functions to Config so that they are easier to find.
Remove helper functions for config values that aren't needed anymore because its safe to directly access the bound field going forward.
Use a fork of viper with a patch for a bug in how we load configuration when a config file is not found. We should still bind to environment variables.
- Fix merging a key into a nil target spf13/viper#1135
- other pr tbd

Checklist

Unit Tests
Documentation
Schema (porter.yaml)

carolynvs · 2021-05-13T16:54:45Z

/azp run porter-integration

azure-pipelines · 2021-05-13T16:54:55Z

Azure Pipelines successfully started running 1 pipeline(s).

cmd/porter/bundle.go

Signed-off-by: Carolyn Van Slyck <[email protected]>

* Add experimental flags to config file. These features are disabled by default. * Add experimental flag for "build-drivers". This allows specifying the build driver, e.g. docker, buildkit, in the config file. * Add driver for buildkit. Signed-off-by: Carolyn Van Slyck <[email protected]>

* Add --experimental flag that accepts a comma separated list of feature flags. Right now we only support "build-drivers". * Refactor porter's config and data structures so that we can't get nil pointer exceptions or try to access the configuration data before it is loaded. * Move helper functions to Config so that they are easier to find. * Remove helper functions for config values that aren't needed anymore because its safe to directly access the bound field going forward. * Use a fork of viper with a patch for a bug in how we load configuration when a config file is not found. We should still bind to environment variables. Signed-off-by: Carolyn Van Slyck <[email protected]>

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs · 2021-05-13T19:12:03Z

/azp run porter-integration

azure-pipelines · 2021-05-13T19:12:13Z

Azure Pipelines successfully started running 1 pipeline(s).

docs/content/custom-dockerfile.md

pkg/claims/claimStorage_test.go

pkg/config/config_test.go

pkg/config/datastore.go

pkg/context/helpers.go

vdice · 2021-05-13T22:18:29Z

pkg/experimental/experimental.go

+)
+
+// ParseFlags converts a list of feature flag names into a bit map for faster lookups.
+func ParseFlags(flags []string) FeatureFlags {


Neat, I'll have to keep this technique in mind.

I could have converted to a map and then used key lookups, but what is nice about this is that it's faster than a map lookup and uses less space too.

vdice · 2021-05-13T22:20:45Z

pkg/porter/build.go

+	return o.bundleFileOptions.Validate(p.Context)
+}
+
+func stringSliceContains(allowedValues []string, value string) bool {


Should we move this to a more generic helpers file/location, or nah?

I think we could use this in a couple places. I just was dithering about naming a package util, and imagined what people would think. 😂

cmd/porter/bundle.go

vdice

Awesome work! Only had minor suggestions.

One other minor issue I was curious about. When I supply an invalid/unsupported build driver, the error isn't as expected:

 $ p build --experimental build-drivers --driver buildx
Copying porter runtime ===>
Copying mixins ===>
Copying mixin exec ===>

Generating Dockerfile =======>
Error: unable to generate Dockerfile: error generating the Dockerfile: error loading default Dockerfile template: open templates/build/buildx.Dockerfile: file does not exist

In other words, I'm wondering if the cli can instead bubble up the specific error that the spec'd build driver isn't supported?

carolynvs · 2021-05-14T13:12:53Z

I have created #1582 to get feedback on auto-build for publish.

I did have code that tried to validate the driver, looks like bad values can still slip through. I'll fix that.

* Fix setting defaults on viper config * Add missing example to custom dockerfile page * Fix comments * Bind --driver to the command flag, not the config struct directly * Add check to AddTestDirectoryFromRoot Signed-off-by: Carolyn Van Slyck <[email protected]>

vdice

One last thing to check. When I supply a bogus driver in Porter's config, I'm not seeing the cli error out on porter build (no flags or env vars set). Do you see the same? I'm using:

experimental = ["build-drivers"]
build-driver = "foo"

I've added a regression test to ensure that when we set a flag for something that falls back to the config, but it also has a flag default set, that the config value is properly bound to the flag. Fixes the edge case of bad config data and the user running porter build without specifying the --driver flag. Previously it was defaulting to the flag's default instead of binding to the config value. I've also tweaked the TestPorter so that it doesn't accidentally load the config and env vars from your dev machine when running tests. Signed-off-by: Carolyn Van Slyck <[email protected]>

Make it easier for tests or downstream consumers of our library to enable experimental flags quickly without having to understand our configuration system. Signed-off-by: Carolyn Van Slyck <[email protected]>

vdice

LGTM!

carolynvs · 2021-05-16T00:57:04Z

Sorry this got a bit weird. I realized that there were some gaps in our configuration scheme exposed by what I wanted to do in this PR. Hopefully we've got enough tests now to cover all the weird cases.

carolynvs · 2021-05-17T13:51:08Z

/azp run porter-integration

azure-pipelines · 2021-05-17T13:51:19Z

Azure Pipelines successfully started running 1 pipeline(s).

* rename build provider * Add new build driver for buildkit * Add experimental flags to config file. These features are disabled by default. * Add experimental flag for "build-drivers". This allows specifying the build driver, e.g. docker, buildkit, in the config file. * Add driver for buildkit. * Add --experimental flag and refactor config/data * Add --experimental flag that accepts a comma separated list of feature flags. Right now we only support "build-drivers". * Refactor porter's config and data structures so that we can't get nil pointer exceptions or try to access the configuration data before it is loaded. * Move helper functions to Config so that they are easier to find. * Remove helper functions for config values that aren't needed anymore because its safe to directly access the bound field going forward. * Use a fork of viper with a patch for a bug in how we load configuration when a config file is not found. We should still bind to environment variables. * Fix review feedback * Fix setting defaults on viper config * Add missing example to custom dockerfile page * Fix comments * Bind --driver to the command flag, not the config struct directly * Add check to AddTestDirectoryFromRoot * Load defaults before applying config to flags I've added a regression test to ensure that when we set a flag for something that falls back to the config, but it also has a flag default set, that the config value is properly bound to the flag. Fixes the edge case of bad config data and the user running porter build without specifying the --driver flag. Previously it was defaulting to the flag's default instead of binding to the config value. I've also tweaked the TestPorter so that it doesn't accidentally load the config and env vars from your dev machine when running tests. * Support setting experimental flags programmatically Make it easier for tests or downstream consumers of our library to enable experimental flags quickly without having to understand our configuration system. Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs changed the base branch from main to v1 May 11, 2021 19:08

carolynvs force-pushed the buildkit branch 3 times, most recently from 724f1b9 to 446bde2 Compare May 13, 2021 12:50

carolynvs commented May 13, 2021

View reviewed changes

cmd/porter/bundle.go Show resolved Hide resolved

carolynvs added 4 commits May 13, 2021 13:34

rename build provider

6859100

Signed-off-by: Carolyn Van Slyck <[email protected]>

wip

24a70d5

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs force-pushed the buildkit branch from 446bde2 to 24a70d5 Compare May 13, 2021 18:35

carolynvs added 2 commits May 13, 2021 13:39

Pin viper

b06241b

Signed-off-by: Carolyn Van Slyck <[email protected]>

damn you go mods

969abe7

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs marked this pull request as ready for review May 13, 2021 19:49

carolynvs requested review from jeremyrickard and vdice as code owners May 13, 2021 19:49

vdice reviewed May 13, 2021

View reviewed changes

Fix review feedback

b06d903

* Fix setting defaults on viper config * Add missing example to custom dockerfile page * Fix comments * Bind --driver to the command flag, not the config struct directly * Add check to AddTestDirectoryFromRoot Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs force-pushed the buildkit branch from ea75e76 to b06d903 Compare May 14, 2021 17:47

vdice reviewed May 14, 2021

View reviewed changes

carolynvs added 2 commits May 14, 2021 17:43

Support setting experimental flags programmatically

8e3fa31

Make it easier for tests or downstream consumers of our library to enable experimental flags quickly without having to understand our configuration system. Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs force-pushed the buildkit branch from dcd4bb9 to 8e3fa31 Compare May 14, 2021 22:44

vdice approved these changes May 14, 2021

View reviewed changes

carolynvs merged commit 3fc2a0e into getporter:v1 May 17, 2021

carolynvs deleted the buildkit branch May 17, 2021 14:28

This was referenced May 22, 2021

Design: Support Cloud Shell #255

Closed

Pluggable Build Driver #361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new build driver for buildkit #1567

Add new build driver for buildkit #1567

carolynvs commented May 8, 2021 •

edited

Loading

carolynvs commented May 13, 2021

azure-pipelines bot commented May 13, 2021

carolynvs commented May 13, 2021

azure-pipelines bot commented May 13, 2021

vdice May 13, 2021

carolynvs May 14, 2021

vdice May 13, 2021

carolynvs May 14, 2021

vdice left a comment

carolynvs commented May 14, 2021

vdice left a comment

vdice left a comment

carolynvs commented May 16, 2021

carolynvs commented May 17, 2021

azure-pipelines bot commented May 17, 2021

Add new build driver for buildkit #1567

Add new build driver for buildkit #1567

Conversation

carolynvs commented May 8, 2021 • edited Loading

What does this change

What issue does it fix

Notes for the reviewer

Checklist

carolynvs commented May 13, 2021

azure-pipelines bot commented May 13, 2021

carolynvs commented May 13, 2021

azure-pipelines bot commented May 13, 2021

vdice May 13, 2021

Choose a reason for hiding this comment

carolynvs May 14, 2021

Choose a reason for hiding this comment

vdice May 13, 2021

Choose a reason for hiding this comment

carolynvs May 14, 2021

Choose a reason for hiding this comment

vdice left a comment

Choose a reason for hiding this comment

carolynvs commented May 14, 2021

vdice left a comment

Choose a reason for hiding this comment

vdice left a comment

Choose a reason for hiding this comment

carolynvs commented May 16, 2021

carolynvs commented May 17, 2021

azure-pipelines bot commented May 17, 2021

carolynvs commented May 8, 2021 •

edited

Loading