-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Fix lifetimes #272
Merged
Merged
Fix lifetimes #272
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thank you! This also fixes a bug where it was impossible to return the |
4 tasks
bors bot
added a commit
to testcontainers/testcontainers-rs
that referenced
this pull request
Feb 19, 2021
261: Upgrade default bitcoin image version to 0.21.0 r=bonomat a=bonomat With the upgrade to 0.21.0 a new startup command was introduced which allows us to execute a script when the startup sequence was completed. We simply echo `bitcoind startup sequence completed.`. This allows us to remove `-debug` which keeps the bitcoind log _clean_. Note: this release also removed the default wallet. 268: Introduce async docker interface and shiplift-backed implementation r=thomaseizinger a=thomaseizinger Blocked by: - [ ] : softprops/shiplift#272 - [ ] : Shiplift release - [x] : #264 - [x] : Open TODOs (see comments) Co-authored-by: Philipp Hoenisch <[email protected]> Co-authored-by: walterh <[email protected]> Co-authored-by: Thomas Eizinger <[email protected]>
10 tasks
elihunter173
added a commit
to elihunter173/shiplift
that referenced
this pull request
Mar 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What did you implement:
After a dance with borrowck, I updated the lifetimes of problematic methods in
lib.rs
where the return's lifetime was unnecessarily tied to some of the inputs. In particular,{Containers, Images, Networks, Services, Volumes}::get()
now return a{Container, Image, Network, Service, Volume}
which is tied to the lifetime of theDocker
they container rather than the lifetime of the{Containers, ...}
which created them. This means we no longer have to create unnecessary locals such asimages
in the following code to avoid the compiler complaining abouttemporary value dropped while borrowed
Instead, we can write the following as you would intuitively expect to work.
Various other methods were also fixed. Generally, anything that returned a stream is now exclusively tied to the lifetime of the
Docker
used to create them (orTransport
fortransport.rs
) rather than the lifetime of both theDocker
and whatever they use as input (e.g.ExecOptions
forContainer::exec()
).Further, I renamed lifetimes from
'a
to'docker
and made some lifetimes explicit to make the code easier to read and understand.Closes: #271
How did you verify your change:
Ran unit tests.
What (if anything) would need to be called out in the CHANGELOG for the next release:
This should be backwards compatible because in every case I made the lifetimes less restrictive. Nevertheless, I updated the changelog to mention it.