Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 oceanbase-ce module #7502
feat: add oceanbase-ce module #7502
Changes from 10 commits
04f4318
d9c4384
76aa6f5
3c2360d
299951a
8eb7836
995ab96
4d10383
4c6922b
e1a303f
66b4592
ae13ed7
5bd35cb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
according to the docs, the default value for
OB_TENAN_NAME
istest
. So we can just haveThere 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.
User can also use 'withEnv' method to set it manually, in this case if we do not check the value, there may be a mistake.
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.
I don't fully understand because the default is
test
and thetenantName = DEFAULT_TEST_TENANT_NAME;
whereDEFAULT_TEST_TENANT_NAME = "test"
. Unless, the right one should useSYSTEM_TENANT_NAME
insteadOnce we resolve this comment I can proceed and merge it
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.
I'm considering this situation: someone use the container class but not set the tenant name with given method
withTenant
, likenew OceanBaseCEContainer("xxx").withEnv("OB_TENANT_NAME", "tc");
. In this case, as the tenantName field is still 'test', a 'test' tenant will be created, not user defined value 'tc'.I'm not sure if we should deal with it. WDYT?
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.
I think that given there is a default value in the image itself. The
withTenant
method and thewithEnv
inconfigure
method can be omitted. If for some reason there is a need to modify it thennew OceanBaseCEContainer(...).withEnv("OB_TENANT_NAME", "something");
that would a similar experience like runningdocker run
command. Otherwise, it would be a little bit confusing.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.
The tenantName field here is required to construct the username used in jdbc connection, so I did not remove the
withTenant
method in previous cleanup. I reconsidered about it and maybe use the default value with necessary description is better.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.
Updated.
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.
I just noticed we were missing a test for this use case and also think a little bit more about it. In the
configure
method we can addgetEnvMap().computeIfAbsent("OB_TENAN_NAME", k -> tenantName != null ? tenantName : DEFAULT_TENANT_NAME)
or justgetEnvMap().computeIfAbsent("OB_TENAN_NAME", k -> tenantName)
and we can keepwithTenantName
. LMK if this can help to the use case described.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.
Yes you are right, we can resolve it by using the envMap, but I think it's not necessary for now.
At present no other
withXXX
method is provided in this container class and all env options are set bywithEnv
, I think it's good and clean. Lets keep it like this.