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(redpanda): Add option for arbitrary bootstrap config #2666

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

oleiman
Copy link
Contributor

@oleiman oleiman commented Jul 22, 2024

What does this PR do?

WithBootstrapConfig adds an arbitrary config kvp to the Redpanda container. Per the name, this config will be interpolated into the generated bootstrap config file, which is particularly useful for configs requiring a restart when otherwise applied to a running Redpanda instance.

Why is it important?

Several Redpanda cluster config properties can be updated live but require a node restart to take effect. The simplest way to expose these to testcontainer consumers is to provide generic access prior to container startup.

An example usage case is an integration test for a particularly memory hungry Wasm Data Transform, where tuning the per core and per function memory limits in a flexible, implementation driven way may be the difference between testing or not testing that function.

Related issues

@oleiman oleiman requested a review from a team as a code owner July 22, 2024 16:22
Copy link

netlify bot commented Jul 22, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 98389fe
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/66b3b360486e0a0008cfc44b
😎 Deploy Preview https://deploy-preview-2666--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.

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

Could you add tests for this new option?

Once there, I think this is LGTM

Cheers!

@oleiman
Copy link
Contributor Author

oleiman commented Jul 23, 2024

Could you add tests for this new option?

Once there, I think this is LGTM

Cheers!

Absolutely. Thanks for the quick review @mdelapenya !

kiview
kiview previously approved these changes Jul 23, 2024
@oleiman
Copy link
Contributor Author

oleiman commented Jul 23, 2024

force push contents: a test

@oleiman
Copy link
Contributor Author

oleiman commented Jul 25, 2024

Hey @mdelapenya - does the test I added look ok to you?

@rockwotj
Copy link

Ping @mdelapenya - I think the concerns here are addressed - thanks in advance!

@mdelapenya
Copy link
Member

I'm on PTO for this week, will take a look then 🙏

@mdelapenya mdelapenya self-assigned this Aug 5, 2024
@mdelapenya mdelapenya added the enhancement New feature or request label Aug 5, 2024
Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Quick pass to help keep this moving along, some issues and questions.

modules/redpanda/mounts/bootstrap.yaml.tpl Outdated Show resolved Hide resolved
modules/redpanda/options.go Show resolved Hide resolved
modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
modules/redpanda/redpanda_test.go Show resolved Hide resolved
modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
@oleiman oleiman force-pushed the redpanda-extra-config branch from 86985f0 to cc28e93 Compare August 6, 2024 03:09
@oleiman
Copy link
Contributor Author

oleiman commented Aug 6, 2024

force push some CR feedback:

  • linter errors
  • template cruft
  • better testify usage

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Thanks for the previous changes, have clarified my previous comment about Body.Close and provided an example, so hope that helps.

modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
@oleiman oleiman force-pushed the redpanda-extra-config branch from cc28e93 to 5899844 Compare August 6, 2024 17:01
@oleiman
Copy link
Contributor Author

oleiman commented Aug 6, 2024

force push contents:

  • correct response body closure in tests
  • s/interface{}/any/

@oleiman
Copy link
Contributor Author

oleiman commented Aug 6, 2024

Thanks for the reviews @stevenh - I think we've addressed all of your comments; please lmk if you have any lingering concerns 🙂

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Just a few asserts remaining if you would be so kind.

As you can see from the example, they aren't an issue until a test actually fails, then they can result it quite hard to understand output as you typically end up with compound errors.

modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
modules/redpanda/redpanda_test.go Outdated Show resolved Hide resolved
@oleiman oleiman force-pushed the redpanda-extra-config branch from 5899844 to 0e1db02 Compare August 6, 2024 21:09
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks all for your work here 🙇

@mdelapenya mdelapenya changed the title Redpanda Module: Add option for arbitrary bootstrap config feat(redpanda): Add option for arbitrary bootstrap config Aug 7, 2024
WithBootstrapConfig adds an arbitrary config kvp to the Redpanda container.
Per the name, this config will be interpolated into the generated bootstrap
config file, which is particularly useful for configs requiring a restart
when otherwise applied to a running Redpanda instance.

Includes a test that sets a couple of restart-requiring cluster configs and
checks a) that the values are reflected in the config state at run time AND
b) that the cluster does not need a restart. (b) demonstrates that the configs
were bootstrapped rather than applied to a live cluster.

Signed-off-by: Oren Leiman <[email protected]>
@oleiman oleiman force-pushed the redpanda-extra-config branch from 0e1db02 to 98389fe Compare August 7, 2024 17:48
@oleiman
Copy link
Contributor Author

oleiman commented Aug 7, 2024

rebase main

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdelapenya mdelapenya merged commit 81007d6 into testcontainers:main Aug 8, 2024
112 checks passed
mdelapenya added a commit that referenced this pull request Aug 9, 2024
* main:
  fix(compose): remove test volumes (#2712)
  chore(mysql): add missing error check in example (#2707)
  chore: remove unused params from defaultPreCreateHook (#2714)
  docs: improve docs for container methods (#2713)
  chore(registry): disable build log (#2711)
  chore: remove obsolete compose version (#2710)
  chore: improve lifecycle errors (#2708)
  docs: add consistent snippets for network creation (#2703)
  test: add retry on system error test (#2687)
  Redpanda Module: Add option for arbitrary bootstrap config (#2666)
  feat(inbucket): expose POP3 and wait for all ports (#2690)
  fix(wait): data race in test (#2698)
  fix(milvus): racy container setup (#2693)
  fix(mongodb): replica test failures (#2699)
  test: racy port creation in port forwarding tests (#2688)
  fix: port forwarding race condition (#2686)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants