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: include PostReady hook, defining proper execution order for container lifecycle hooks #1922

Merged

Conversation

mdelapenya
Copy link
Collaborator

@mdelapenya mdelapenya commented Nov 16, 2023

What does this PR do?

This PR internally changes how the container lifecycle hooks are executed. Before these changes:

  • default hooks (logging, config modifiers, copy files and wait strategies) were executed first
  • user-defined hooks were executed after the default ones.

It could be OK, but it is not allowing users to perform tasks related to the warm-up of a container:

  • create a dir so that copied files are able to live in a directory that the underlying Docker image does not have.
  • execute code in a startup command, and wait for it as part of the wait strategies

With the changes in this PR, we are updating the execution order to:

  1. execute default PRE hooks first
  2. execute user-defined PRE hooks second
  3. execute user-defined POST hooks first
  4. execute default POST hooks second

With this order, we make sure that users can define their stuff at the right place in the container life cycle.

In order to achieve this order, we are combining default and user-defined hooks into one single hook, and that will be the one finally added to the container request. We have added unit tests to verify that the order is honoured.

To enrich the experience, we are defining a new container lifecycle event: Ready, which happens right after the PostStart event. This Ready event defines PostReadies hooks only. This new event will represent the execution of the wait strategies, which is the Testcontainers representation for Readiness.

Finally, given the PR touches the container lifecycles, we are refining the log messages in the default logging hooks, always using ✅ for the default post hooks.

Why is it important?

Define a consistent container life cycl, allowing users to act before the default hooks.

Related issues

@mdelapenya mdelapenya requested a review from a team as a code owner November 16, 2023 11:26
@mdelapenya mdelapenya self-assigned this Nov 16, 2023
Copy link

netlify bot commented Nov 16, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 8dd85e8
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/65cd5c8cce78cc000876d938
😎 Deploy Preview https://deploy-preview-1922--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Nov 16, 2023
@pablochacin
Copy link
Contributor

pablochacin commented Nov 16, 2023

execute default PRE hooks first
execute user-defined PRE hooks second
execute user-defined POST hooks first
execute default POST hooks second
With this order, we make sure that users can define their stuff at the right place in the container life cycle.

I think this still doesn't make clear how this hook execution order related to the container's readiness. I just found it by looking at the code and realizing the waitfor is executed as a post-start hook. This wasn't obvious to me. I suggest we make this more explicit in some way.

@mdelapenya
Copy link
Collaborator Author

I suggest we make this more explicit in some way.

You mean in the docs and in this PR description? PTAL at https://deploy-preview-1922--testcontainers-go.netlify.app/features/creating_container/#lifecycle-hooks, the Info section

@mdelapenya
Copy link
Collaborator Author

I think this still doesn't make clear how this hook execution order related to the container's readiness.

In that sense I believe we need to always run the default wait strategies before the user-defined post-start hooks. Otherwise we can see a module trying to run commands even before the readyness (defined by the wait strategies) is not complete.

I'm tempted to always prepend the wait strategies to the post-start hooks. Wdyt? @kiview we have already discussed about this today

@pablochacin
Copy link
Contributor

In that sense I believe we need to always run the default wait strategies before the user-defined post-start hooks. Otherwise we can see a module trying to run commands even before the readyness (defined by the wait strategies) is not complete.

This is how this works now and precisely the problem I mentioned in the slack.

The changes introduced in this PR change that and allow running a post-strart hook before the wait condition, and then use a wait condition that verifies the effects of such hook.

maybe I'm missing something here, but what you propose seems to take us back to the current behavior

@pablochacin
Copy link
Contributor

I suggest we make this more explicit in some way.

You mean in the docs and in this PR description? PTAL at https://deploy-preview-1922--testcontainers-go.netlify.app/features/creating_container/#lifecycle-hooks, the Info section

Documentation. In the proposed changes you mention this in a info note regarding the default hooks, but I think the definition of Post-start hook should be more specific, because "started" and "ready" (post wait condition meets) are in my opinion two different things and make create confusion.

@mdelapenya
Copy link
Collaborator Author

@pablochacin I've updated the PR and description to reflect the new Ready lifecycle event, with its own Pre/Post hooks. PTAL 🙏

* main: (24 commits)
  chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (testcontainers#1982)
  chore(deps): bump github.com/twmb/franz-go in /modules/redpanda (testcontainers#1973)
  chore(deps): bump google.golang.org/api from 0.152.0 to 0.153.0, cloud.google.com/go/bigtable from 1.20.0 to 1.21.0 and cloud.google.com/go/spanner from 1.53.0 to 1.53.1 in /modules/gcloud (testcontainers#1983)
  chore(deps): bump github.com/aws/aws-sdk-go and github.com/aws/aws-sdk-go-v2 in /modules/localstack (testcontainers#1981)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.0.1 to 6.0.4 (testcontainers#1974)
  feat: add module to support Microsoft SQL Server (testcontainers#1969)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.10 to 3.23.11 (testcontainers#1943)
  chore(deps): bump golang.org/x/mod in /modules/kafka (testcontainers#1956)
  chore(deps): bump golang.org/x/sys from 0.13.0 to 0.15.0 (testcontainers#1944)
  chore(deps): bump golang.org/x/text and golang.org/x/mod from 0.13.0 to 0.14.0 in /modulegen (testcontainers#1968)
  chore(deps): bump go.mongodb.org/mongo-driver in /modules/mongodb (testcontainers#1960)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#1952)
  chore(deps): bump github.com/elastic/go-elasticsearch/v8 from 8.10.1 to 8.11.1 and golang.org/x/mod from 0.13.0 to 0.14.0 in /modules/elasticsearch (testcontainers#1967)
  chore(deps): bump github.com/aws/aws-sdk-go and github.com/aws/aws-sdk-go-v2 in /modules/localstack (testcontainers#1953)
  chore(deps): bump actions/github-script from 6.4.1 to 7.0.1 (testcontainers#1949)
  chore(deps): bump github.com/IBM/sarama in /modules/kafka (testcontainers#1955)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1961)
  chore(deps): bump github.com/compose-spec/compose-go from 1.20.0 to 1.20.2 and github.com/docker/compose/v2 from 2.23.0 to 2.23.3 in /modules/compose (testcontainers#1966)
  chore(deps): bump google.golang.org/api from 0.143.0 to 0.152.0 and cloud.google.com/go/spanner from 1.50.0 to 1.53.0 in /modules/gcloud (testcontainers#1965)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.0.1 to 6.0.4 (testcontainers#1934)
  ...
Copy link
Contributor

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

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

LGTM.

* main: (25 commits)
  chore(deps): bump github.com/dvsekhvalnov/jose2go in /modules/pulsar (testcontainers#2136)
  fix: skip-host-cache option removed in latest MySQL 8.3.0 version (testcontainers#2130)
  chore: skip assertions for Docker Rootless (testcontainers#2135)
  pin Docker images version (testcontainers#2129)
  enable golangci-lint for examples (testcontainers#2128)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#2098)
  enable golangci-lint for redis module (testcontainers#2126)
  Go install gotestsum and golangci-lint  (testcontainers#2127)
  improve OSSF score (testcontainers#2125)
  chore: run make lint on new modules (testcontainers#2122)
  enable golangci-lint for pulsar (testcontainers#2121)
  lint: enable testifylint (testcontainers#2120)
  chore(deps): bump github.com/nats-io/nats.go in /modules/nats (testcontainers#2094)
  chore(deps): bump golang.org/x/sys from 0.15.0 to 0.16.0 (testcontainers#2104)
  Revert "chore(deps): bump actions/upload-artifact from 3.1.3 to 4.0.0 (testcontainers#2088)"
  chore(deps): bump actions/upload-artifact from 3.1.3 to 4.0.0 (testcontainers#2088)
  chore(deps): bump cloud.google.com/go/spanner from 1.54.0 to 1.55.0, google.golang.org/api from 0.154.0 to 0.156.0 in /modules/gcloud (testcontainers#2115)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/config from 1.25.10 to 1.26.3, github.com/aws/aws-sdk-go from 1.48.13 to 1.49.19 in /modules/localstack (testcontainers#2114)
  chore(deps): bump github.com/docker/go-connections from 0.4.0 to 0.5.0 (testcontainers#2113)
  Adding mockserver module (testcontainers#2085)
  ...
* main: (33 commits)
  feat (postgres): support for creating and restoring Snapshots (testcontainers#2199)
  fix: apply volume options only to volumes (testcontainers#2201)
  redpanda/test: add admin client call (testcontainers#2200)
  chore(deps): bump cloud.google.com/go/spanner from 1.55.0 to 1.56.0 in /modules/gcloud, cloud.google.com/go/pubsub from 1.35.0 to 1.36.1 in /modules/gcloud, cloud.google.com/go/bigquery from 1.57.1 to 1.58.0 in /modules/gcloud (testcontainers#2197)
  chore(deps): bump github.com/docker/docker from 25.0.1+incompatible to 25.0.2+incompatible (testcontainers#2196)
  fix: go doc reference broken image (testcontainers#2195)
  Add Support for WASM Transforms to Redpanda Module (testcontainers#2170)
  feat(modules.clickhouse): Add zookeeper for clickhouse clusterization (testcontainers#1995)
  redpanda: allow using SASL and TLS together (testcontainers#2140)
  chore: do not panic in testable examples (testcontainers#2193)
  fix: all mounts should contain the testcontainers labels (testcontainers#2191)
  [redpanda] sasl test for wrong mechanism (testcontainers#2048)
  fix: deprecate BindMounts correctly (testcontainers#2190)
  chore(docker_mounts): stop doing misleading logging (testcontainers#2178)
  fix: Add HTTPStrategy WithForcedIPv4LocalHost To Fix Docker Port Map (testcontainers#1775)
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#2162)
  feat(modules.cockroachdb) Adds cockroachdb module (testcontainers#2131)
  chore(deps): bump golang.org/x/crypto in /modules/minio (testcontainers#2161)
  chore(deps): bump golang.org/x/crypto in /modules/openldap (testcontainers#2165)
  chore(deps): bump github.com/google/uuid from 1.5.0 to 1.6.0 (testcontainers#2169)
  ...
@mdelapenya mdelapenya merged commit c906e65 into testcontainers:main Feb 15, 2024
80 checks passed
@mdelapenya mdelapenya deleted the user-defined-lifecycle-hooks branch February 15, 2024 10:44
@mdelapenya mdelapenya added feature New functionality or new behaviors on the existing one and removed chore Changes that do not impact the existing functionality labels Feb 15, 2024
@mdelapenya mdelapenya changed the title chore: define proper execution order for container lifecycle hooks feat: include PostReady hook, defining proper execution order for container lifecycle hooks Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Properly define the execution order for container lifecycle hooks
2 participants