Skip to content

Conversation

@original-brownbear
Copy link
Contributor

@original-brownbear original-brownbear added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 v6.7.0 labels Jan 25, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@original-brownbear original-brownbear changed the title Retry minio port Use TestFixturesPlugin to Run Minio in Tests Jan 25, 2019
}
minioDockerfile.parentFile.mkdirs()
minioDockerfile.text = "FROM minio/minio:RELEASE.2019-01-23T23-18-58Z\n" +
"RUN mkdir -p /minio/data/${s3PermanentBucket}\n" +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to dynamically build the Dockerfile here since we need to create this directory in the container somehow to force the existence of a bucket.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have hard coded values for bucket, access and secret keys in case we use minio so the test always runs ? Could also be in a follow up, but that way we would not need a docker file at all as we can just include the hard-coded values in the docker compose configuration and mount a volume that already has a directory for the bucket. I don't think this is a blocker to merge this PR, but would be good to have this test running with the regular build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to use local fs volume mounts though? I'd rather not, that's gonna be a little annoying for environments (like my personal CI :D) that use Docker in Docker or a remote Docker daemon?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this will work in docker based local CI right now and the tests will just be skipped. I'm not sure we can avoid the volume mounts for all fixtures anyway, but maybe we can create the dirs as part of the start command ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atorok

I'm not sure this will work in docker based local CI right now

Works for me :D You just gotta mount the docker socket right and it all works fine :)

I'm not sure we can avoid the volume mounts for all fixtures anyway

I'd vote for maybe just adding some way of copying dirs in and (if needed) out of the containers via Gradle (tasks). That would be completely portable.
Also, it would avoid some foreseeable weirdness, where we'd create root owned files in the volumes through the containers and then can't delete them in ./gradlew clean :)

maybe we can create the dirs as part of the start command?

annoyingly enough we can't do that I think, because of the weird way the official container sets up the environment (we're only supplying the args to command right now).
Also, for #37393 I'd love to be able to be a little more flexible here and maybe create multiple containers with different bucket names.

@alpar-t
Copy link
Contributor

alpar-t commented Jan 25, 2019

thanks @original-brownbear for taking this up! I'm glad to see that code in build.gradle go.

@original-brownbear
Copy link
Contributor Author

@atorok thanks for taking a look, changed the task dependency relations as requested and removed the container shutdown now :)

Copy link
Contributor

@alpar-t alpar-t left a comment

Choose a reason for hiding this comment

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

LGTM

@original-brownbear
Copy link
Contributor Author

@atorok thanks!

@original-brownbear original-brownbear merged commit be6bdab into elastic:master Jan 25, 2019
@original-brownbear original-brownbear deleted the retry-minio-port branch January 25, 2019 11:56
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jan 25, 2019
* Disable Minio fixture and tests that require it whne fixtures are disabled or Docker is not available
* Relates elastic#37852
original-brownbear added a commit that referenced this pull request Jan 25, 2019
* Disable Minio fixture and tests that require it when fixtures are disabled or Docker is not available
* Relates #37852
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jan 29, 2019
* Use TestFixturesPlugin to Run Minio in Tests

* Closes elastic#37680
* Closes elastic#37783
original-brownbear added a commit that referenced this pull request Jan 29, 2019
* Use TestFixturesPlugin to Run Minio in Tests

* Closes #37680
* Closes #37783
talevy pushed a commit to talevy/elasticsearch that referenced this pull request Feb 15, 2019
* Use TestFixturesPlugin to Run Minio in Tests

* Closes elastic#37680
* Closes elastic#37783
talevy pushed a commit to talevy/elasticsearch that referenced this pull request Feb 19, 2019
* Disable Minio fixture and tests that require it when fixtures are disabled or Docker is not available
* Relates elastic#37852
@jpountz jpountz added v6.6.3 and removed v6.6.2 labels Mar 7, 2019
talevy added a commit that referenced this pull request Mar 11, 2019
* Use TestFixturesPlugin to Run Minio in Tests  (#37852)

* Use TestFixturesPlugin to Run Minio in Tests

* Closes #37680
* Closes #37783

* Fix S3 Repository ITs When Docker is not Available (#37878)

* Disable Minio fixture and tests that require it when fixtures are disabled or Docker is not available
* Relates #37852

* add explicit composeUp dependsOn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >test Issues or PRs that are addressing/adding tests v6.6.3 v6.7.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants