Improve passing custom values as build arguments #2641

carolynvs · 2023-03-21T14:46:00Z

What does this change

Currently porter passes all custom values defined in porter.yaml as build arguments when using docker to build the invocation image.

custom:
  app:
    version: 1.2.3

The above porter.yaml results in porter passing --build-arg CUSTOM_APP_VERSION=1.2.3

We have some users who are storing longer values in the custom map and have managed to hit the max argument length on their system. To address this, I am no longer passing all custom values as build arguments, and only pass them when the Dockerfile declares a build argument with the same name. In the example above, we only pass the app version when the Dockerfile declares ARG CUSTOM_APP_VERSION. In this users's case that's enough to avoid the problem.

In addition to the change above, I have also added a max length check on build arguments so that we can detect that it may be too long and provide a better error message. When we let the large value get passed to docker the error is simply "exit code 1" and we don't get any context into what failed. This will ensure that long values are caught before calling docker and telling the user to save the value in a file, and then read it back out in the Dockerfile instead.

What issue does it fix

Closes #2633

Notes for the reviewer

N/A

Checklist

Did you write tests?
Did you write documentation?
Did you change porter.yaml or a storage document record? Update the corresponding schema file.
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

Comment with /azp run test-porter-release if a magefile or build script was modified
Comment with /azp run porter-integration if it's a non-trivial PR

Currently porter passes all custom values defined in porter.yaml as build arguments when using docker to build the invocation image. ```yaml custom: app: version: 1.2.3 ``` The above porter.yaml results in porter passing --build-arg CUSTOM_APP_VERSION=1.2.3 We have some users who are storing longer values in the custom map and have managed to hit the max argument length on their system. To address this, I am no longer passing all custom values as build arguments, and only pass them when the Dockerfile declares a build argument with the same name. In the example above, we only pass the app version when the Dockerfile declares `ARG CUSTOM_APP_VERSION`. In this users's case that's enough to avoid the problem. In addition to the change above, I have also added a max length check on build arguments so that we can detect that it may be too long and provide a better error message. When we let the large value get passed to docker the error is simply "exit code 1" and we don't get any context into what failed. This will ensure that long values are caught before calling docker and telling the user to save the value in a file, and then read it back out in the Dockerfile instead. Signed-off-by: Carolyn Van Slyck <[email protected]>

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

arschles

Just some nit-pick comments and suggestions. Overall, LGTM!

docs/content/bundle/custom-dockerfile.md

pkg/build/buildkit/buildx.go

* Use code blocks such as `porter build --build-arg` instead of "--build-arg flag on porter build". It reads more cleanly when someone is just reading the markdown and not the rendered HTML * Split function arguments onto seprate lines since they are long lines Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs · 2023-03-21T20:31:03Z

Thanks @arschles! I've updated the code to be a bit more readable, especially when viewing the raw markdown. The --build-arg is a Hugo thing, it let's me use a double hypen without it being turned into a dash. But I can see how it's super confusing in markdown and not all previewers support the escape code.

joshuabezaleel · 2023-03-22T09:02:54Z

pkg/build/buildkit/buildx_test.go

+ BuildArgs: []string{"BIG_BUILD_ARG=" + oversizedArg},
+ }
+ _, err := b.determineBuildArgs(ctx, m, opts)
+ assert.ErrorContains(t, err, "BIG_BUILD_ARG is longer than the max")


is there a specific need here that we use assert when the rest is using require?

Yup! We are slowly reworking the integration tests to be written like the smoke tests, longer tests that validate a number of scenarios to limit how much setup is required for reach test.

Right now the integration tests are kind of written like unit tests, and they try to set a small set of assertions. This isn't working well for us in Porter because the cost of test setup (building and publishing a bundle) is very high compared to the cost of the rest of the test. By testing a "workflow" or series of test cases in a single test, for example checkout the airgap test, we can check more assertions with less setup cost.

The problem with long tests though is that it's easy to have the first thing fail, and nothing else after that is going to pass (because each part of the test sets up the next part, such as installing a bundle, then upgrading it). By using require, we make the test stop on the first error. It then fails more quickly (instead of running when we know everything after that point will most likely fail). It also helps make it easier to troubleshoot since you only see the most relevant failure (the first one) instead of 20 assert messages, and you have to scroll to figure out which one is the cause of it all.

This is a great question, I'll add it to a README in the tests directory so it's easier to discover.

Got it, make sense, thank you lots for the explanation, Carolyn!

joshuabezaleel · 2023-03-22T09:04:25Z

tests/integration/build_test.go

+ test.Chdir(test.TestDir)
+
+ // Use a unique version for appversion so that docker doesn't cache the result and not print the value used in the Dockerfile
+ appversion := fmt.Sprintf("app.versio=%d", time.Now().Unix())


small typo: i guess it should be app.version instead of app.versio

Thanks for catching that, it actually pointed out a problem with how I was doing the test, so I've cleaned it up a bit more.

I was being too clever trying to deal with cachebusting the dockerfile and it totally wasn't necesary. I've simplified the test and fixed the typo Signed-off-by: Carolyn Van Slyck <[email protected]>

This is follow up to getporter#2641 (comment) which documents why we use require in integration/smoke tests instead of assert. Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs · 2023-03-22T20:34:09Z

@joshuabezaleel I've fixed the test, can you take another look?

* Document why we use require in long tests This is follow up to #2641 (comment) which documents why we use require in integration/smoke tests instead of assert. Signed-off-by: Carolyn Van Slyck <[email protected]> * Apply suggestions from code review Co-authored-by: Aaron Schlesinger <[email protected]> Signed-off-by: Carolyn Van Slyck <[email protected]> --------- Signed-off-by: Carolyn Van Slyck <[email protected]> Co-authored-by: Aaron Schlesinger <[email protected]>

joshuabezaleel · 2023-03-25T07:56:17Z

@carolynvs Apologize for the late reply.
Thank you for always taking the extra miles in making Porter an accessible and welcoming project to contributors, Carolyn, in which you didn't just kindly explain in length about the reasoning but also sent a PR to update the docs for others to read, really appreciate that from the very beginning.

The changes looks awesome to me, all good 👍

* Document why we use require in long tests This is follow up to getporter#2641 (comment) which documents why we use require in integration/smoke tests instead of assert. Signed-off-by: Carolyn Van Slyck <[email protected]> * Apply suggestions from code review Co-authored-by: Aaron Schlesinger <[email protected]> Signed-off-by: Carolyn Van Slyck <[email protected]> --------- Signed-off-by: Carolyn Van Slyck <[email protected]> Co-authored-by: Aaron Schlesinger <[email protected]>

* Improve passing custom values as build arguments Currently porter passes all custom values defined in porter.yaml as build arguments when using docker to build the invocation image. ```yaml custom: app: version: 1.2.3 ``` The above porter.yaml results in porter passing --build-arg CUSTOM_APP_VERSION=1.2.3 We have some users who are storing longer values in the custom map and have managed to hit the max argument length on their system. To address this, I am no longer passing all custom values as build arguments, and only pass them when the Dockerfile declares a build argument with the same name. In the example above, we only pass the app version when the Dockerfile declares `ARG CUSTOM_APP_VERSION`. In this users's case that's enough to avoid the problem. In addition to the change above, I have also added a max length check on build arguments so that we can detect that it may be too long and provide a better error message. When we let the large value get passed to docker the error is simply "exit code 1" and we don't get any context into what failed. This will ensure that long values are caught before calling docker and telling the user to save the value in a file, and then read it back out in the Dockerfile instead. Signed-off-by: Carolyn Van Slyck <[email protected]> * Document max length on --custom field Signed-off-by: Carolyn Van Slyck <[email protected]> * Improve readability * Use code blocks such as `porter build --build-arg` instead of "--build-arg flag on porter build". It reads more cleanly when someone is just reading the markdown and not the rendered HTML * Split function arguments onto seprate lines since they are long lines Signed-off-by: Carolyn Van Slyck <[email protected]> * Fix build test I was being too clever trying to deal with cachebusting the dockerfile and it totally wasn't necesary. I've simplified the test and fixed the typo Signed-off-by: Carolyn Van Slyck <[email protected]> --------- Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs force-pushed the selective-build-args branch 3 times, most recently from 3a42500 to 09ce26c Compare March 21, 2023 17:47

carolynvs force-pushed the selective-build-args branch from 09ce26c to 823e224 Compare March 21, 2023 18:08

carolynvs marked this pull request as ready for review March 21, 2023 18:49

carolynvs requested review from VinozzZ, sgettys and bdegeeter as code owners March 21, 2023 18:49

carolynvs requested a review from vdice March 21, 2023 19:08

Document max length on --custom field

4354d79

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

arschles reviewed Mar 21, 2023

View reviewed changes

joshuabezaleel reviewed Mar 22, 2023

View reviewed changes

Fix build test

2f515f5

I was being too clever trying to deal with cachebusting the dockerfile and it totally wasn't necesary. I've simplified the test and fixed the typo Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs mentioned this pull request Mar 22, 2023

Document why we use require in long tests #2643

Merged

arschles approved these changes Mar 22, 2023

View reviewed changes

carolynvs merged commit 7d35c81 into getporter:main Mar 27, 2023

carolynvs deleted the selective-build-args branch March 27, 2023 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve passing custom values as build arguments #2641

Improve passing custom values as build arguments #2641

carolynvs commented Mar 21, 2023

arschles left a comment

carolynvs commented Mar 21, 2023

joshuabezaleel Mar 22, 2023 •

edited

Loading

carolynvs Mar 22, 2023

carolynvs Mar 22, 2023

carolynvs Mar 22, 2023

joshuabezaleel Mar 25, 2023

joshuabezaleel Mar 22, 2023

carolynvs Mar 22, 2023

carolynvs commented Mar 22, 2023

joshuabezaleel commented Mar 25, 2023 •

edited

Loading

Improve passing custom values as build arguments #2641

Improve passing custom values as build arguments #2641

Conversation

carolynvs commented Mar 21, 2023

What does this change

What issue does it fix

Notes for the reviewer

Checklist

Reviewer Checklist

arschles left a comment

Choose a reason for hiding this comment

carolynvs commented Mar 21, 2023

joshuabezaleel Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

carolynvs Mar 22, 2023

Choose a reason for hiding this comment

carolynvs Mar 22, 2023

Choose a reason for hiding this comment

carolynvs Mar 22, 2023

Choose a reason for hiding this comment

joshuabezaleel Mar 25, 2023

Choose a reason for hiding this comment

joshuabezaleel Mar 22, 2023

Choose a reason for hiding this comment

carolynvs Mar 22, 2023

Choose a reason for hiding this comment

carolynvs commented Mar 22, 2023

joshuabezaleel commented Mar 25, 2023 • edited Loading

joshuabezaleel Mar 22, 2023 •

edited

Loading

joshuabezaleel commented Mar 25, 2023 •

edited

Loading