-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Move Cassandra env variable setup to protected method #5991
Conversation
With testcontainers#2830 we added some environment variable setup to the constructor in CassandraContainer. This causes problems in scenarios where users customize the environment themselves or customize the values in cassandra.yaml, so move this init to a protected method to facilitate overriding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks again @akhaku ! Just left a comment to be reviewed by the team.
withEnv("CASSANDRA_SNITCH", "GossipingPropertyFileSnitch"); | ||
withEnv("JVM_OPTS", "-Dcassandra.skip_wait_for_gossip_to_settle=0 -Dcassandra.initial_token=0"); | ||
withEnv("HEAP_NEWSIZE", "128M"); | ||
withEnv("MAX_HEAP_SIZE", "1024M"); | ||
withEnv("CASSANDRA_ENDPOINT_SNITCH", "GossipingPropertyFileSnitch"); | ||
withEnv("CASSANDRA_DC", DEFAULT_LOCAL_DATACENTER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC those env were added in order to improve cassandra startup. I think we can apply those only if the image name is cassandra
. @kiview WDYT?
It can be also applied for library/cassandra
for those using a local registry. but we can do something like new CassandraContainer("library/cassandra:3.2.1").applyDefaultEnvVars()
. Just throwing an idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yeah no strong opinions on the mechanism apart from maybe not coupling it to the image name directly, seems a little brittle and might still cause problems for anyone who copies their own cassandra.yaml in into the container. Moving it to an overridable protected method was the least obtrusive way to allow turning it off but making it explicitly opt-in is fine too! I suppose we'll wait for Kevin to weigh in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddumelendez I think our container implementations should always assume a compatible image and not bind specific integration to the name. Else, it will become more of a hassle, once users provide their own image (also in this case we should assume a working image as default IMO).
@akhaku TBH, I don't understand the PR fully. Isn't it enough, if you do setEnv(new ArrayList()
if you want to remove the defaults here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for that, I didn't realize that I could empty out the environment map like that. That workaround is fine by me!
Moving it to a protected method makes it easer to extend (e.g. I could call super.initEnv
in my overridden method before or after adding my own env vars) but calling setEnv
with an empty list works for my use case! I'll go ahead and decline this PR for now.
Also, only just realized that I forgot to add a call to initEnv
in the constructor, I meant to have that there so it doesn't change existing behavior 🤦 sorry that probably caused some confusion too.
Closing this PR because as @kiview suggested, |
With #2830 we added some environment variable setup to the constructor in CassandraContainer. This causes problems in scenarios where users customize the environment themselves or customize the values in cassandra.yaml, so move this init to a protected method to facilitate overriding.