feat: Add support to seed data when using RunBigQueryContainer #2523
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
modules/gcloud/bigquery_test.go
Outdated
| "github.com/testcontainers/testcontainers-go" | ||
| "github.com/testcontainers/testcontainers-go/modules/gcloud" | ||
| ) | ||
|
|
||
| func ExampleRunBigQueryContainer() { | ||
| func TestBigQueryContainer(t *testing.T) { |
There was a problem hiding this comment.
We would like to keep some testable examples, as they will land in the pkg.go.dev docs. If you have doubts on when creating a real test or a testable example, please use a testable example when you want to show case how to use the module. Else, a regular test is fine.
modules/gcloud/bigquery.go
Outdated
| for _, opt := range opts { | ||
| if err := opt.Customize(&req); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Mmm I think the applyOptions call will already do this. Please see https://github.com/testcontainers/testcontainers-go/pull/2523/files#diff-658ed21fe179e3c4810748c731c20b75757c269dd8ce30f0bf98ce31f2addd24R91-R103
There was a problem hiding this comment.
@mdelapenya it wasn't working before without this and that appears to be due to needing to update
req.Cmd = []string{"--project", settings.ProjectID} to req.Cmd = append(req.Cmd, "--project", settings.ProjectID) so that the options added to the cmd are not removed
There was a problem hiding this comment.
Let me test this locally, from this branch. Will come back to you later
There was a problem hiding this comment.
Indeed, with the addition of the YAML data file, now it's possible to have different settings contributing to the CMD. So it's fine if we append.
My only concern is now related to the usage of the gcloud options across all the emulators 🤔 Do you know if all of them would support passing this YAML file? Else, anybody could add this functional option to e.g. Firestore or Pubsub, and expect something else. I'm not an expert in those services, so you probably have more insights.
There was a problem hiding this comment.
Good point, I myself have not used the other emulators but would assume this data yaml functionality would and should only apply for the bigquery emulator. I'm currently not sure of a great way to prevent this in the other gcloud emulators. I think the data.yaml option should be ignored though due to how that CMD is being set.
req.Cmd = []string{...}
It looks like the spanner container might be the only one that differs. It does not have req.Cmd = []string{...}
| if err != nil { | ||
| log.Fatalf("failed to run container: %v", err) | ||
| } |
There was a problem hiding this comment.
suggestion: use require.NoError to simplify, more below
| if err != nil { | |
| log.Fatalf("failed to run container: %v", err) | |
| } | |
| require.NoError(t, err) |
| defer func() { | ||
| if err := bigQueryContainer.Terminate(ctx); err != nil { | ||
| log.Fatalf("failed to terminate container: %v", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
suggestion: use testcontainers.CleanupContainer to simplify and ensure correct cleanup.
| if err != nil { | ||
| log.Fatalf("failed to iterate: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
bug: left over output comments below.
|
|
||
| // Output: | ||
| // [30] | ||
| expectedValue := int64(30) | ||
| actualValue := val[0] | ||
| if expectedValue != actualValue { |
There was a problem hiding this comment.
suggestion: use require.Equal or EqualValue
| @@ -41,12 +41,14 @@ func newGCloudContainer(ctx context.Context, port int, c testcontainers.Containe | |||
| } | |||
|
|
|||
| type options struct { | |||
| ProjectID string | |||
| ProjectID string | |||
| DataYamlFile string | |||
There was a problem hiding this comment.
bug: this doesn't seem to be ever used.
| // WithDataYamlFile seeds the Bigquery project for the GCloud container. | ||
| func WithDataYamlFile(dataYamlFile string) testcontainers.CustomizeRequestOption { |
There was a problem hiding this comment.
suggestion: remove the duplication between function name and paramater
|
|
||
| container, err := testcontainers.GenericContainer(ctx, req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| spannerContainer, err := newGCloudContainer(ctx, 9050, container, settings) | ||
| bigqueryContainer, err := newGCloudContainer(ctx, 9050, container, settings) |
There was a problem hiding this comment.
Looks like this needs a rebase as there has been some work to ensure this correctly returns the container even on error.
What does this PR do?
Updates the
RunBigQueryContainerto optionally use the emulator'sdata-from-yamloption, allowing users to seed the container with data.Why is it important?
This give the user of the
RunBigQueryContainerthe option to seed test data on the container creation/startup.Related issues