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

Add param of withBindVolumn for EtcdClusterExtension #1092

Closed

Conversation

liangyuanpeng
Copy link
Contributor

I use remote docker for testcontainer to run unit test and i just got the error:

Caused by: com.github.dockerjava.api.exception.InternalServerErrorException: Status 500: {"message":"invalid volume specification: 'C:\\Users\\Administrator\\AppData\\Local\\Temp\\jetcd_test_etcd0_8631704060335352785:/data.etcd:rw,z'"}

So i add param of bindVolumn for this case that do not need volumn.

Signed-off-by: Lan Liang <gcslyp@gmail.com>
@lburgazzoli
Copy link
Collaborator

lburgazzoli commented Sep 18, 2022

It would be nice to understand why the volume mount fails, ideally this should not be something you should be able to disable as it may be required by the jetcd container

@liangyuanpeng
Copy link
Contributor Author

liangyuanpeng commented Sep 25, 2022

It would be nice to understand why the volume mount fails,

Run unit test on windows, and use remote docker container from linux, the voulme is C:\\Users\\Administrator\\xxx:/data.etcd:rw,z, this format can not use by docker container. So just got the error when container start.

ideally this should not be something you should be able to disable as it may be required by the jetcd container

Absolutely right, so still create volum for etcd test container by default, User need to use withBindVolumn(false) for disable volumn when they want to this.

Copy link
Collaborator

@lburgazzoli lburgazzoli left a comment

Choose a reason for hiding this comment

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

some minor changes are needed

@@ -46,6 +46,7 @@ public static class Builder {
private boolean ssl = false;
private List<String> additionalArgs;
private Network network;
private boolean bindVolumn = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • the default value is usually set in the constructor
  • maybe use a better name that describe the intention i.e. bindDataDirectory

@@ -203,7 +211,9 @@ public void start() {
@Override
public void close() {
super.close();
deleteDataDirectory(dataDirectory);
if(bindVolumn) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this check may not be needed as if the data directory os null, then the method won't do anything

@@ -79,6 +80,11 @@ public EtcdContainer withClusterToken(String clusterToken) {
this.clusterToken = clusterToken;
return self();
}

public EtcdContainer withBindVolumn(boolean bindVolumn){
this.bindVolumn=bindVolumn;
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably a checkstyle violation, please reformat the code

@@ -57,6 +58,7 @@ public EtcdClusterImpl(
.withClusterToken(clusterName)
.withSll(ssl)
.withAdditionalArgs(additionalArgs)
.withBindVolumn(bindVolumn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

must be formatted

@github-actions
Copy link

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Nov 25, 2022
@lburgazzoli
Copy link
Collaborator

@liangyuanpeng can you fix the findings ?

@IlyasYOY
Copy link
Contributor

@lburgazzoli is this PR up to date?

I opened a #1123, I guess they are almost identical.

I have all code formatter + tests there. This feature is very important to me.

Thanks!

@lburgazzoli
Copy link
Collaborator

@lburgazzoli is this PR up to date?

I opened a #1123, I guess they are almost identical.

I have all code formatter + tests there. This feature is very important to me.

Thanks!

Merged, THX !

@lburgazzoli
Copy link
Collaborator

Done in #1123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants