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.

Sign up for GitHub

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

Jump to bottom

libcnb-test should pass --trust-builder to pack build #407

Closed
edmorley opened this issue Jun 17, 2022 · 0 comments · Fixed by #409
Closed

libcnb-test should pass --trust-builder to pack build #407

edmorley opened this issue Jun 17, 2022 · 0 comments · Fixed by #409
Assignees
@edmorley
Labels
faster tests Things that improve the runtime of tests libcnb-test

Comments

@edmorley
Copy link
Member

edmorley commented Jun 17, 2022 •
edited
Loading

Pack has the concept of trusted builders:
https://buildpacks.io/docs/tools/pack/concepts/trusted_builders/

Any of the builders on Pack's "suggested builders" list (which will include the Heroku stacks) are automatically trusted, however if tests either use a different builder, or if Pack hasn't yet been updated to include a new builder (such as our newly created heroku/builder:22), then that builder won't be trusted.

If a builder is not trusted, then

  1. The build stages are each run in a separate container (which is slightly slower)
  2. The log output changes to have a [<stage>] prefix on every line.

Example logs for a trusted builder:

===> DETECTING
heroku/procfile 1.0.1
===> RESTORING
===> BUILDING
 
[Discovering process types]
Procfile declares types -> web, worker
===> EXPORTING
Adding layer 'launch.sbom'
...

Example logs for an untrusted builder:

===> DETECTING
[detector] heroku/procfile 1.0.1
===> RESTORING
===> BUILDING
[builder] 
[builder] [Discovering process types]
[builder] Procfile declares types -> web, worker
===> EXPORTING
[exporter] Adding layer 'launch.sbom'
...

In the latter there are now line prefixes like [detector] and [builder].

If multi-line libcnb-test log output assertions are used (such as in heroku/buildpacks-procfile#75), tests that passed on one machine (where the builder was trusted) can then fail when run on another machine where the Pack config doesn't have that builder marked as trusted.

I believe we should get libcnb-test to pass --trust-builder to its pack build calls (which overrides the trusted builders list just for that build), in order to:

  • normalise build log output in tests across environments
  • avoid having to add pack config trusted-builders add ZZZZ boilerplate to all of our CI configs
  • speed up test execution (eg: trusting the builder makes the procfile-cnb integration tests take 25% less time on my local machine; albeit this is a best case, since these are simple/fast tests)

This should be safe to do, since:

  • libcnb-test doesn't publish images or pass in credentials (one of the concerns of using an untrusted builder)
  • projects using libcnb-test will have chosen/vetted their test images (so it's a bit different from the "end user experimenting with random builders they found on the internet" case)
  • by compiling/running a Rust libcnb.rs-using project on your local machine you are already running a bunch of untrusted code, so you either trust the author of that buildpack or you don't.

GUS-W-11312254.
@edmorley edmorley added libcnb-data faster tests Things that improve the runtime of tests labels Jun 17, 2022
edmorley added a commit to heroku/buildpacks-procfile that referenced this issue Jun 17, 2022
@edmorley
Improve integration test coverage
855661b 
Now that libcnb-test has added support for more methods of invoking
the built containers (see heroku/libcnb.rs#366),
we are able to add integration tests for more than just the `web` process
handling. 

Whilst the unit tests for this repository are very comprehensive, much of
the end buildpack behaviour depends on pack/lifecycle/the buildpack API
version, which aren't tested by the unit tests. For example buildpack API 0.6
changed the default process handling in a breaking way.

This PR:
- Adds five additional integration tests.
- Drops the usage of `ureq` / making HTTPS requests to the `web` process,
  in favour of a simpler `echo` / logs output based approach. The only reason
  the former approach was used was because of the "valid Procfile but not valid
  YAML" case, however that is now tested via a simpler `echo` fixture that includes
  the offending `"key: value"` substring, rather than  the `socat` example command.
- Drops usage of `tempfile` / manually writing files in the unit tests, in favour of
  reading files from the fixtures directory.
- Uses `indoc!` to assert against multi-line log output strings, to make the end
  output more clearly visible from the test, and also reduce the chance of false
  positives (for example, `Procfile declares types -> web` would previously
  successfully match against `Procfile declares types -> web, something-else`
  since there was no end of line marker).
- Uses the new `ignore = "reason"` support in Rust 1.61, to make it
  clearer why tests are being skipped when running `cargo test`.
- Adds `heroku/builder:22` to the trusted builders list, since otherwise
  it slows down builds, and also means log output has additional prefixes,
  which mess up the multi-line assertions. Longer term we will add this
  builder to Pack's built-in trusted builders list, and make `libcnb-test`
  pass `--trust-builder` to the `pack build` command:
  heroku/libcnb.rs#407

GUS-W-11311966.
@edmorley edmorley mentioned this issue Jun 17, 2022
Merged
@edmorley edmorley self-assigned this Jun 17, 2022
edmorley added a commit that referenced this issue Jun 17, 2022
@edmorley
libcnb-test: Pass --trust-builder to pack build
4039100 
In order to:
- Normalise build log output in tests across environments.
- Avoid having to add `pack config trusted-builders add ZZZZ`
  boilerplate to all of our CI configs.
- Speed up test execution.

See the issue description for more details.

Fixes #407.
GUS-W-11312254.
@edmorley edmorley mentioned this issue Jun 17, 2022
Merged
edmorley added a commit that referenced this issue Jun 20, 2022
@edmorley
libcnb-test: Pass --trust-builder to pack build (#409)
15c2cd1 
In order to:
- Normalise build log output in tests across environments.
- Avoid having to add `pack config trusted-builders add ZZZZ`
  boilerplate to all of our CI configs.
- Speed up test execution.

See the issue description for more details.

Fixes #407.
GUS-W-11312254.
edmorley added a commit to heroku/buildpacks-procfile that referenced this issue Jun 20, 2022
@edmorley
Improve integration test coverage (#75)
9a9ba97 
Whilst the unit tests for this repository are very comprehensive, much
of the end buildpack behaviour depends on Pack CLI / `lifecycle` / the
buildpack API version, which aren't tested by the unit tests.

For example Buildpack API 0.6 changed the default process handling in a
breaking way:
https://github.com/buildpacks/spec/releases/tag/buildpack%2Fv0.6

Now that `libcnb-test` has added support for more methods of invoking
the built containers (see heroku/libcnb.rs#366),
we are able to add integration tests for more than just a single `web`
process example. 

This PR:
- Adds five additional integration tests.
- Drops the usage of `ureq` / making HTTPS requests to the `web` process,
  in favour of a simpler `echo` / logs output based approach. The only
  reason the former approach was used was because of the "valid Procfile
  but not valid YAML" case (see 741bbc0),
  however that is now tested via a simpler `echo` fixture that includes
  the offending `"key: value"` substring, rather than  the `socat`
  example command.
- Drops usage of `tempfile` / manually writing files in the unit tests,
  in favour of reading files from the fixtures directory.
- Uses `indoc!` to assert against multi-line log output strings, to make
  the end output more clearly visible from the test, and also reduce the
  chance of false positives (for example, `Procfile declares types -> web`
  would previously successfully match against
  `Procfile declares types -> web, something-else` since there was no end
  of line marker).
- Uses the new `ignore = "reason"` support in Rust 1.61, to make it
  clearer why (integration) tests are being skipped when running
  `cargo test`.
- Remove the redundant `.buildpacks(vec![BuildpackReference::Crate])`
  calls, since the current buildpack is the default when using
  `libcnb-test`'s `run_test()`.
- Removes `RUST_BACKTRACE=1` from the Circle CI configs, since in
  general the backtraces are noisy (making it harder to read the actual
  error) and add little value over the file+line number that's already
  output in the standard error message. In rare scenarios where the
  backtrace is actually useful, it can be enabled locally, or on a
  Circle CI run with a debugging commit pushed to the PR etc.
- Adds `heroku/builder:22` to the trusted builders list, since otherwise
  it slows down builds, and also means log output has additional prefixes,
  which mess up the multi-line assertions. Longer term we will add this
  builder to Pack's built-in trusted builders list, and also make
  `libcnb-test` pass `--trust-builder` to the `pack build` command:
  heroku/libcnb.rs#407

GUS-W-11311966.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@edmorley edmorley

Labels
faster tests Things that improve the runtime of tests libcnb-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

libcnb-test: Pass --trust-builder to pack build heroku/libcnb.rs
1 participant
@edmorley