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

Entrypoint customization #174

Merged
merged 7 commits into from
Jul 8, 2020
Merged

Entrypoint customization #174

merged 7 commits into from
Jul 8, 2020

Conversation

tumdum
Copy link
Contributor

@tumdum tumdum commented Jun 12, 2020

Extend Image so that it is possible to customize entrypoint used by image.

Copy link
Collaborator

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thank you :)

Can you also include a changelog entry please?

let docker = clients::Cli::default();
let msg = images::generic::WaitFor::message_on_stdout("server is ready");

let generic = images::generic::GenericImage::new("tumdum/simple_web_server:latest")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this image going to be supported for longer?

I wouldn't want to depend on an image for our tests that breaks or is removed eventually :)

It would be nice to depend on one of the standard images like alpine for these tests.

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've created it just for this pull request, source is here: https://git.sr.ht/~ttt/simple_web_server

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

If it is specifically designed for this purpose, it would be nice if we could colocate the source with the test.

Do you mind including it in this repository? I was thinking of using a cargo buildscript to build (and tag) the container at build time so it is available for the test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thomaseizinger Building container in build script can lead to a situation where after building, user removes all local containers and later reruns tests only to find out that they no longer pass. Is this an desired behaviour?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is a very rare situation no?

By default, build scripts are run on every invocation anyway unless we make use of some of the "change detection" mechanisms.
Given that container builds through docker are idempotent, this should be relatively fast.

We would have to opt out from the buildscript in non-test cases. No sure if that is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

move all the tests to a separate crate and include the buildscript in there :)

You want to have this change done in this pull request?

Copy link
Collaborator

Choose a reason for hiding this comment

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

move all the tests to a separate crate and include the buildscript in there :)

You want to have this change done in this pull request?

I saw you licensed the image source as A-GPL. To my knowledge, that is not compatible with the MIT or Apache2 license in this repository.
If you are ok with me taking the source of the docker image and putting it into this repository despite the license then we can merge the PR as is! I guess we would have to drop A-GPL then? Not sure about this to be honest.

If you are keen to introduce a testing crate as described above then even better but it is not a requirement from my side :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are ok with me taking the source of the docker image and putting it into this repository despite the license then we can merge the PR as is! I guess we would have to drop A-GPL then? Not sure about this to be honest.

I'm completely fine with that - you can use that code as if it was licensed under either MIT or Apache2 license.

Do you need me to change license in that repo or is my written permission here enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is good enough, thank you :)

I am happy to merge this as is then!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your contribution and patience :)

tests/images.rs Outdated Show resolved Hide resolved
src/clients/cli.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Collaborator

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 8, 2020

Build succeeded:

@bors bors bot merged commit b00703c into testcontainers:dev Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants