Allow starting a container without using the default process type

[Malax]

Allow starting a container without using the default process type. This enables us to test images with multiple process types, process types that take arguments as well as starting a container with a custom command.

To make the custom command feature useful, the current logs of the container are now accessible via the ContainerContext so you can match against it in tests.

Updated execd example to no longer use a bogus sleep 3600 default process as a workaround.

Changelog additions:

• Allow starting a container without using the default process type. Removed PrepareContainerContext::start, added PrepareContainerContext::start_with_default_process, PrepareContainerContext::start_with_default_process_args, PrepareContainerContext::start_with_process, PrepareContainerContext::start_with_process_args, PrepareContainerContext::start_with_shell_command (#366)
• Add ContainerContext::logs and ContainerContext::logs_follow to access the logs of the container. Useful when used in conjunction with the new PrepareContainerContext::start_with_shell_command method to get the shell command output. (#366)
• Replaced container_context::ContainerExecResult with log::LogOutput which is now shared with ContainerContext::logs and ContainerContext::logs_follow. (#366)

Closes GUS-W-10539911
Closes #309

[schneems]

FWIW this was a meh upgrade experience. I got this error:

error[E0599]: no method named `start` found for mutable reference `&mut PrepareContainerContext<'_>` in the current scope
  --> tests/integration_test.rs:23:18
   |
23 |                 .start(|container| {
   |                  ^^^^^ method not found in `&mut PrepareContainerContext<'_>`

And so I check the readme which shows this code:

// In $CRATE_ROOT/tests/integration_test.rs
use libcnb_test::{IntegrationTest, BuildpackReference, assert_contains};

#[test]
fn test() {
    IntegrationTest::new("heroku/buildpacks:20", "test-fixtures/app")
        .buildpacks(vec![
            BuildpackReference::Other(String::from("heroku/openjdk")),
            BuildpackReference::Crate,
        ])
        .run_test(|context| {
            assert_contains!(context.pack_stdout, "---> Maven Buildpack");
            assert_contains!(context.pack_stdout, "---> Installing Maven");
            assert_contains!(context.pack_stdout, "---> Running mvn package");

            context.prepare_container().expose_port(12345).start(|container| {
                assert_eq!(
                    call_test_fixture_service(
                        container.address_for_port(12345).unwrap(),
                        "Hagbard Celine"
                    )
                    .unwrap(),
                    "enileC drabgaH"
                );
            });
        });
}

fn call_test_fixture_service(addr: std::net::SocketAddr, payload: &str) -> Result<String, ()> {
   unimplemented!()
}

So it looks like start method does exist, but there is some other problem.

I spelunk to find the context.rs file and see this change as the most recent history. Then kinda intuit from the commit name and the all the start_ methods that I should use start_with_default_process instead.

My thoughts:

• Readme needs to be tested as this is invalid code that will not compile
• Just because we're statically typed doesn't mean interface changes won't be intuitive. I'm wanting some kind of a "this is deprecated, use this instead" experience which I would expect from even a language like Ruby. Granted there you have to manually do it, and not everyone does. Maybe there are some better UX patterns or macros or crates or something that exist for when we rename something (not just methods, structs and implementations as well).

Overall it's not a HUGE deal. Just wanted to give stream-of-concious and in-the-moment feedback as I think it's valuable. Whether we choose to act on it or not.

[edmorley]

@schneems Thank you for the feedback!

Agree the libcnb-test README should be fixed - the code examples in repo root README are already tested in CI - this one is supposed to be too, but maybe the [test] annotation is interfering with that? I've filed #403.

These breaking changes were mentioned in the changelog (and the changelog gets inlined into Dependabot PRs for example, though you won't have such a PR since the Ruby Rust changes are not merged). However I agree this requires someone reading the changelog, so best if there is a more automatic solution.

Rust offers the ability to show such a warning, which we could use for future breaking changes:
https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute

That all said, it's worth bearing in mind that much of libcnb (particularly libcnb-test) is still at the very early pre-v1.0.0 stage (our team's buildpacks are the early adopters/guinea pigs for them), so we expect there to be more breaking changes/rough edges now, than there will be in the future. The changelog is there to try and make this slightly less confusing, so I would recommend reading it in the future :-)

[schneems]

That all said, it's worth bearing in mind that much of libcnb (particularly libcnb-test) is still at the very early pre-v1.0.0 stage (our team's buildpacks are the early adopters/guinea pigs for them), so we expect there to be more breaking changes/rough edges now, than there will be in the future. The changelog is there to try and make this slightly less confusing, so I would recommend reading it in the future :-)

I think there's two components for me 1) is a pure technical "can we do this" and the other is the implementation side. You've answered the "can we do this" thank you.

I think the second component is more of a cultural "should we do this". I certainly understand the cost/time tradeoff. I personally fall into habits where I "play" much like I "practice". I.e. I tend to do things always or never. If I don't make a habit of going through deprecation cycles then when it's really critical to do so I'll likely forget. I believe you have more self control as well as awareness and also are good at reviewing PRs. I need more black and white policies as well as automated tooling.

I do want to push back on the "it's just us" narrative. While that's true for now, the longer we act that way, the more hostile the codebase will be to adoption by others (self fulfilling prophecy). Also even "just us" is a hefty cost/time tradeoff. I spent maybe 60 min (perhaps rounding up) investigating this. If we multiply by number of team mebers then taking an extra hour to reduce multiplicative time spent might not always come out to be the best investment but it's an investment and does have payback.

The other thing of value I gain from choosing to use deprecation cycles is that it informs the implementation and also forces us to think about "after this PR is merged how will we deprecate or remove this code if it's no longer needed". In the same way I believe in shipping tests with code as it's written as the tests will inform the API. Not so dissimilar to making plans to deprecate and remove a stack before it's even released.

Coming from Ruby the main thing's I'm hoping to gain from Rust are improved maintainability and part of that is library/ecosystem deprecations.

I don't think we disagree in a large number of these areas I am somewhat typing to get my thoughts out and clear on the subject. I'm curious in talking more about the path forward for some of this stuff regarding when a good time to flip the switch from "not now" to "now" might be. Alternatively, maybe we could make me the "deprecation czar" (or something) and I can do some of the legwork of trying to figure out the deprecation/upgrade path on breaking changes.