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(wait): tls strategy #2896

Merged
merged 5 commits into from
Nov 26, 2024
Merged

feat(wait): tls strategy #2896

merged 5 commits into from
Nov 26, 2024

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Nov 25, 2024

Extract TLS certificate wait strategy into a dedicated wait type so it can be reused.

Use embed to simplify wait test loading of certs.

Copy link

netlify bot commented Nov 25, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 58ba86c
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6745ca96433cde00086cb7bd
😎 Deploy Preview https://deploy-preview-2896--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.

@stevenh stevenh changed the title feat: tls strategy and strategy walk feat(wait): tls strategy Nov 25, 2024
Extract TLS certificate wait strategy into a dedicated wait type so it
can be reused.

Use embed to simplify wait test loading of certs.
@stevenh stevenh marked this pull request as ready for review November 26, 2024 10:31
@stevenh stevenh requested a review from a team as a code owner November 26, 2024 10:31
@stevenh stevenh requested a review from mdelapenya November 26, 2024 10:31
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! I added some comments, not blockers, but want to discuss them before accepting it.

Also, do you foresee any usage for this strategy in the existing modules? Elasticsearch, Cockroachdb, and more use TLS certs.

wait/tls_test.go Show resolved Hide resolved

// ForTLSCert returns a CertStrategy that will add a Certificate to the [tls.Config]
// constructed from PEM formatted certificate key file pair in the container.
func ForTLSCert(certPEMFile, keyPEMFile string) *TLSStrategy {
Copy link
Member

@mdelapenya mdelapenya Nov 26, 2024

Choose a reason for hiding this comment

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

question: do you think it would be interesting to provide a way for consumers to forget about generating certs and the library build them on the fly? Something like wait.ForTLSTestCert? The library would generate the file under the hood.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep love that idea, something for a follow up PR.

@stevenh
Copy link
Collaborator Author

stevenh commented Nov 26, 2024

LGTM, thanks! I added some comments, not blockers, but want to discuss them before accepting it.

Also, do you foresee any usage for this strategy in the existing modules? Elasticsearch, Cockroachdb, and more use TLS certs.

Great stuff, yes this is extracted from the cockroachdb PR but it could be used with any module where TLS is needed.

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.

Last minute review of the docs site, and I found the new page is not discoverable.

Because this is a new page, we need to add it to the mkdocs.yml descriptor:

Please see https://deploy-preview-2896--testcontainers-go.netlify.app/features/wait/introduction/

This is not automated, although there is room for improving that.

Improve docs adding more snippets as suggested by review.
@stevenh
Copy link
Collaborator Author

stevenh commented Nov 26, 2024

Last minute review of the docs site, and I found the new page is not discoverable.

Because this is a new page, we need to add it to the mkdocs.yml descriptor:

Please see https://deploy-preview-2896--testcontainers-go.netlify.app/features/wait/introduction/

This is not automated, although there is room for improving that.

Yep spotted that adding the changes you requested, had a assumed it was automated, something to mention in the contributing guide.

mdelapenya
mdelapenya previously approved these changes Nov 26, 2024
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, only the mkdocs is missing, but other than that, it's ready

Added docs link to mkdocs.yml for generation.
Clarify why a cert should not be included in a production image.
@stevenh stevenh requested a review from mdelapenya November 26, 2024 13:18
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!

@stevenh stevenh merged commit 5baaa6c into main Nov 26, 2024
122 checks passed
@stevenh stevenh deleted the feat/wait-tls branch November 26, 2024 15:25
@mdelapenya mdelapenya added the enhancement New feature or request label Nov 26, 2024
@mdelapenya mdelapenya self-assigned this Nov 26, 2024
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Dec 5, 2024
* main:
  fix: container binds syntax (testcontainers#2899)
  refactor(cockroachdb): to use request driven options (testcontainers#2883)
  chore(deps): bump actions/setup-go from 5.0.0 to 5.1.0 (testcontainers#2904)
  chore(deps): bump ossf/scorecard-action from 2.3.1 to 2.4.0 (testcontainers#2903)
  chore(deps): bump test-summary/action from 2.3 to 2.4 (testcontainers#2902)
  feat(wait): strategy walk (testcontainers#2895)
  feat(wait): tls strategy (testcontainers#2896)
mdelapenya added a commit to mtellis2/testcontainers-go that referenced this pull request Dec 11, 2024
* main: (234 commits)
  chore(ci): add Github labels based on PR title (testcontainers#2914)
  chore(gha): Use official setup-docker-action (testcontainers#2913)
  chore(ci): enforce conventional commits syntax in PR titles (testcontainers#2911)
  feat(nats): WithConfigFile - pass a configuration file to nats server (testcontainers#2905)
  chore: enable implicit default logger only in testing with -v (testcontainers#2877)
  fix: container binds syntax (testcontainers#2899)
  refactor(cockroachdb): to use request driven options (testcontainers#2883)
  chore(deps): bump actions/setup-go from 5.0.0 to 5.1.0 (testcontainers#2904)
  chore(deps): bump ossf/scorecard-action from 2.3.1 to 2.4.0 (testcontainers#2903)
  chore(deps): bump test-summary/action from 2.3 to 2.4 (testcontainers#2902)
  feat(wait): strategy walk (testcontainers#2895)
  feat(wait): tls strategy (testcontainers#2896)
  docs: better contribution guidelines (testcontainers#2893)
  fix(influxdb): Respect custom waitStrategy (testcontainers#2845)
  fix: only upload to sonar on ubuntu-latest (testcontainers#2891)
  fix: build artifact name properly (testcontainers#2890)
  fix: do not run sonar upload when ryuk is disabled (testcontainers#2889)
  fix: update GH actions for uploading/downloading artifacts (testcontainers#2888)
  feat(ci): Enable master moby with rootless (testcontainers#2880)
  fix(redpanda): temporary file use
  ...
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.

2 participants