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

fix!: update ports on container restart #562

Closed

Conversation

stormshield-gt
Copy link

fix #561

@DDtKey DDtKey changed the title fix container restart fix: container restart Apr 6, 2024
@DDtKey DDtKey changed the title fix: container restart fix: update ports on container restart Apr 6, 2024
@DDtKey DDtKey changed the title fix: update ports on container restart fix!: update ports on container restart Apr 6, 2024
@@ -222,8 +222,9 @@ where
self.docker_client.stop(&self.id)
}

pub fn start(&self) {
pub fn start(&mut self) {
Copy link
Collaborator

@DDtKey DDtKey Apr 6, 2024

Choose a reason for hiding this comment

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

It's definitely breaking change fix, because public interface has been changed.

In order to avoid this we could consider interior mutability for ports instead.

However, it might be not really safe to introduce interior mutability here.
Because another usage of the container might need to be aware of the restart and port changes, thus mut reference provides some guarantees to us regarding API.

Copy link
Author

Choose a reason for hiding this comment

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

As this crate hasn't reached 1.0 yet maybe we should just do the breaking change and wait for the next release (0.16.0) to publish it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, general interface is gonna be refactored anyway with breaking changes.

So I'm ok with that

@DDtKey DDtKey changed the title fix!: update ports on container restart fix: update ports on container restart Apr 6, 2024
@DDtKey DDtKey changed the title fix: update ports on container restart fix!: update ports on container restart Apr 6, 2024
@DDtKey
Copy link
Collaborator

DDtKey commented Apr 16, 2024

Actually it looks like we just don't need to cache the ports, see #392 (comment)

I'm going to refactor this as part of new version (which addresses #563 and #386).

Thank you very much for the contribution 🙏
But I'm going to close the PR, sorry for that

@DDtKey DDtKey closed this Apr 16, 2024
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.

Mapped ports are not updated after container restart
2 participants