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: Add Cassandra module #1726

Merged
merged 7 commits into from
Oct 11, 2023
Merged

Conversation

anilsenay
Copy link
Contributor

What does this PR do?

This PR adds Cassandra module for go-testcontainers

Why is it important?

It is important as part of a task of #636 to support Cassandra modules too.

Related issues

Follow-ups

Cassandra docker image does not provide a configuration from environment variables for authentication. It allows all connections by default without auth. To enable authentication and set a user, you should provide it in config yaml file or it may be ran by cqlsh command in container to execute query as an alternative. As a follow-up improvement, authentication options could be added somehow.

@anilsenay anilsenay requested a review from a team as a code owner October 8, 2023 21:45
@netlify
Copy link

netlify bot commented Oct 8, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 70992e2
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/652660b7829c580008babab8
😎 Deploy Preview https://deploy-preview-1726--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.

@@ -51,7 +51,7 @@ If you need to set a different database, and its credentials, you can use the `W

#### Init Scripts

If you would like to do additional initialization in the Postgres container, add one or more `*.sql`, `*.sql.gz`, or `*.sh` scripts to the container request with the `WithInitScript` function.
If you would like to do additional initialization in the Postgres container, add one or more `*.sql`, `*.sql.gz`, or `*.sh` scripts to the container request with the `WithInitScripts` function.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I saw typo in postgres readme, I wanted to fix this typo in this pr also.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do it in another PR as it is outside the current scope.

for _, script := range scripts {
cf := testcontainers.ContainerFile{
HostFilePath: script,
ContainerFilePath: "/" + filepath.Base(script),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cassandra docker image does not support /docker-entrypoint-initdb.d/ so I put script files in root directory and in line 105 they will be executed via cqlsh or sh command by file extension. Is this a valid solution? What do you think?

@@ -51,7 +51,7 @@ If you need to set a different database, and its credentials, you can use the `W

#### Init Scripts

If you would like to do additional initialization in the Postgres container, add one or more `*.sql`, `*.sql.gz`, or `*.sh` scripts to the container request with the `WithInitScript` function.
If you would like to do additional initialization in the Postgres container, add one or more `*.sql`, `*.sql.gz`, or `*.sh` scripts to the container request with the `WithInitScripts` function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do it in another PR as it is outside the current scope.

"path/filepath"
"strings"

"github.com/docker/go-connections/nat"
Copy link
Contributor

Choose a reason for hiding this comment

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

Gci shall require a new line here.
Which IDE are you using ?

Copy link
Contributor Author

@anilsenay anilsenay Oct 9, 2023

Choose a reason for hiding this comment

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

Im using vscode but also i checked in GoLand, it did not do anything or warn about it. But I added new line manually. Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdelapenya , shall an issue be created ? This is the second module where the linting is not applied.
@anilsenay , Is golangci-lint installed on your machine ?
In vscode, are you using the workspace configuration from here ?
In your settings, do you have this:

"go.lintTool": "golangci-lint"

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could run golangci-lint when a module is generated, but instead of using the binary (see Makefile) calling the right Docker image to avoid having to install it on every developer's machine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not every machine has docker, on windows for exemple, it's not installed by default.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but installing docker is easier than installing a binary for linting. In any case, I'm biased from my past experience in a release engineering team, where we dockerised everything to run smoothly on the CI 😅

"fmt"
"path/filepath"

"github.com/gocql/gocql"
Copy link
Contributor

Choose a reason for hiding this comment

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

There shall be a new line here too

Also revert typo fix in postgres document while its not scope of this pr
@mdelapenya mdelapenya self-assigned this Oct 9, 2023
@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Oct 9, 2023
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.

Great job with the new module! Thanks for contributing it!

I left a few comments, the most important for me is the one leveraging the container lifecycle hooks using startup commands.

Other than than, feels look good to me. Once we address them, I think it will be ready to be merged

Thanks!

modules/cassandra/cassandra.go Show resolved Hide resolved
modules/cassandra/cassandra.go Show resolved Hide resolved
modules/cassandra/cassandra.go Outdated Show resolved Hide resolved
modules/cassandra/cassandra.go Outdated Show resolved Hide resolved
modules/cassandra/cassandra_test.go Outdated Show resolved Hide resolved
modules/cassandra/cassandra_test.go Outdated Show resolved Hide resolved
@mdelapenya mdelapenya added the hacktoberfest Pull Requests accepted for Hacktoberfest. label Oct 9, 2023
Comment on lines +1 to +18
package cassandra

import (
"strings"
)

type initScript struct {
File string
}

func (i initScript) AsCommand() []string {
if strings.HasSuffix(i.File, ".cql") {
return []string{"cqlsh", "-f", i.File}
} else if strings.HasSuffix(i.File, ".sh") {
return []string{"/bin/sh", i.File}
}
return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdelapenya hey, can you review this again? I add this to the separated file as "executable" but I need help with the naming convention because there is no example except rabbitmq/types_test.go. I think we should decide on good naming for both file names and structs for this executable usage for the sake of example to other modules

Copy link
Member

Choose a reason for hiding this comment

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

I think initScript, not exported, is fine. I thought about the following names:

  • cassandraCommand: no because it includes the package namespace, and no again because calling the .AsCommand() method will come with duplication.
  • script: no because it's very simple
  • executable: no because it's very abstract
  • initScript: in the context of Cassandra, I think this is the one that makes more sense.

mdelapenya
mdelapenya previously approved these changes Oct 10, 2023
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.

Let's rename the executable.go file, and once there. we are good to go.

In any case, LGTM!

modules/cassandra/executable.go Show resolved Hide resolved
Comment on lines +1 to +18
package cassandra

import (
"strings"
)

type initScript struct {
File string
}

func (i initScript) AsCommand() []string {
if strings.HasSuffix(i.File, ".cql") {
return []string{"cqlsh", "-f", i.File}
} else if strings.HasSuffix(i.File, ".sh") {
return []string{"/bin/sh", i.File}
}
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

I think initScript, not exported, is fine. I thought about the following names:

  • cassandraCommand: no because it includes the package namespace, and no again because calling the .AsCommand() method will come with duplication.
  • script: no because it's very simple
  • executable: no because it's very abstract
  • initScript: in the context of Cassandra, I think this is the one that makes more sense.

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! I took the license to fix a typo in a comment to not load you again with more rework.

Thanks again for your had work here! Much appreciated! 💪

@mdelapenya
Copy link
Member

Merging, as last commit is a change in a comment.

@mdelapenya mdelapenya merged commit 1cb3391 into testcontainers:main Oct 11, 2023
17 checks passed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Oct 13, 2023
* main:
  docs: Fix typo and mention the relevant function name in doc (testcontainers#1745)
  DOCKER_HOST var typo (testcontainers#1743)
  feat: Add Cassandra module (testcontainers#1726)
  K6 module (testcontainers#1721)
  Rancher Desktop instructions (testcontainers#1724)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.8 to 3.23.9 (testcontainers#1720)
  chore(deps): bump urllib3 from 2.0.5 to 2.0.6 (testcontainers#1718)
  chore(deps): bump github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1714)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#1704)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1713)
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Oct 16, 2023
* main: (137 commits)
  Fix wrong module names (testcontainers#1776)
  docs: add default options to k6 module (testcontainers#1744)
  fix race condition in Test_StartStop (testcontainers#1700)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/{service/s3,credentials,config} in /modules/localstack (testcontainers#1773)
  chore(deps): bump cloud.google.com/go/{datastore,bigtable,spanner} in /modules/gcloud (testcontainers#1774)
  chore(deps): bump golang.org/x/net from 0.15.0 to 0.17.0 (testcontainers#1772)
  docs: Fix typo and mention the relevant function name in doc (testcontainers#1745)
  DOCKER_HOST var typo (testcontainers#1743)
  feat: Add Cassandra module (testcontainers#1726)
  K6 module (testcontainers#1721)
  Rancher Desktop instructions (testcontainers#1724)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.8 to 3.23.9 (testcontainers#1720)
  chore(deps): bump urllib3 from 2.0.5 to 2.0.6 (testcontainers#1718)
  chore(deps): bump github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1714)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#1704)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1713)
  chore(deps): bump github.com/nats-io/nats.go in /modules/nats (testcontainers#1705)
  chore(deps): bump cloud.google.com/go/firestore from 1.12.0 to 1.13.0, google.golang.org/api from 0.142.0 to 0.143.0 and cloud.google.com/ge, google.golang.org/api from 0.142.0 to 0.143.0 and cloud.google.com/go/bigquery from 1.53.0 to 1.55 in /modules/gcloud (testcontainers#1716)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 from 1.39.0 to 1.40.0 and github.com/aws/aws-sdk-go from 1.45.15 to 1.45.19 in /modules/localstack (testcontainers#1717)
  chore: prepare for next minor development cycle (0.26.0)
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Oct 16, 2023
* main: (137 commits)
  Fix wrong module names (testcontainers#1776)
  docs: add default options to k6 module (testcontainers#1744)
  fix race condition in Test_StartStop (testcontainers#1700)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/{service/s3,credentials,config} in /modules/localstack (testcontainers#1773)
  chore(deps): bump cloud.google.com/go/{datastore,bigtable,spanner} in /modules/gcloud (testcontainers#1774)
  chore(deps): bump golang.org/x/net from 0.15.0 to 0.17.0 (testcontainers#1772)
  docs: Fix typo and mention the relevant function name in doc (testcontainers#1745)
  DOCKER_HOST var typo (testcontainers#1743)
  feat: Add Cassandra module (testcontainers#1726)
  K6 module (testcontainers#1721)
  Rancher Desktop instructions (testcontainers#1724)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.8 to 3.23.9 (testcontainers#1720)
  chore(deps): bump urllib3 from 2.0.5 to 2.0.6 (testcontainers#1718)
  chore(deps): bump github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1714)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#1704)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1713)
  chore(deps): bump github.com/nats-io/nats.go in /modules/nats (testcontainers#1705)
  chore(deps): bump cloud.google.com/go/firestore from 1.12.0 to 1.13.0, google.golang.org/api from 0.142.0 to 0.143.0 and cloud.google.com/ge, google.golang.org/api from 0.142.0 to 0.143.0 and cloud.google.com/go/bigquery from 1.53.0 to 1.55 in /modules/gcloud (testcontainers#1716)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 from 1.39.0 to 1.40.0 and github.com/aws/aws-sdk-go from 1.45.15 to 1.45.19 in /modules/localstack (testcontainers#1717)
  chore: prepare for next minor development cycle (0.26.0)
  ...
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 hacktoberfest Pull Requests accepted for Hacktoberfest.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants