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

Replace all Shiplift usages with Bollard #323

Merged
merged 2 commits into from
Jan 17, 2022
Merged

Replace all Shiplift usages with Bollard #323

merged 2 commits into from
Jan 17, 2022

Conversation

andrey-yantsen
Copy link
Contributor

Judging by the low activity in shiplift (last commit on 1 Sep 2021, quite a few unmerged PRs after September and lack of response in the maintenance status clarification request softprops/shiplift#321), I think it's a good time to switch to a more actively maintained crate.

I picked Ballard because of the more active repo than docker-api-rs. And I liked the idea with the code generation from the API :)

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!

concept ACK but I think we can omit the changelog entry. See comment!

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -2,7 +2,7 @@ use crate::{
core::{env, env::GetEnvValue, logs::LogStream, ports::Ports, ContainerState, Docker, WaitFor},
Container, Image, ImageArgs, RunnableImage,
};
use shiplift::rep::ContainerDetails;
use bollard::models::ContainerInspectResponse;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we would inline this struct, then we could feature-flag bollard on the experimental. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting idea. Not sure about inlining the struct, though — we'd have to track the changes in the API, and it's not fun :) We could use bollard-stubs (it's the crate where bollard stores the autogenerated models) inside CLI; it should be enough. Are you OK with it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, that is much better! Certainly more lightweight!

`bollard-stubs` is a crate with autogenerated models for the Docker API.
Previously, we were using models embedded in `bollard` inside CLI, while
the client itself wasn't required. Now we're making `bollard` an
optional dependency and including it only when we need the
heavy-lifting, i.e. the client logic.

The internal implementation of the CLI client uses the models from
`bollard-stubs` from now on.
@andrey-yantsen
Copy link
Contributor Author

Anyway, I pushed the usage of bollard-stubs and made bollard dependency optional (enabling it only for the experimental feature) :) If you are not happy with it — I'm open to suggestions.

P.S. I just discovered that the switch to bollard fixes #279. There're a couple more shiplift-related issues: #270 and #271; I haven't checked those yet.

@thomaseizinger thomaseizinger linked an issue Jan 17, 2022 that may be closed by this pull request
@thomaseizinger
Copy link
Collaborator

Thank you very much for this effort. Nice that this fixes #279!

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 17, 2022

Build succeeded:

@bors bors bot merged commit c533d0f into testcontainers:dev Jan 17, 2022
@andrey-yantsen andrey-yantsen deleted the replace-shiplift-with-bollard branch January 17, 2022 22:44
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.

Not working on Windows 10
2 participants