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

Stop docker containers on linux #18

Merged
merged 15 commits into from
May 14, 2024

Conversation

0kate
Copy link
Contributor

@0kate 0kate commented May 6, 2023

Description of the changes

  1. Additional support to stop docker containers by specifying port number.
  • add Killable trait for abstraction layer to implement stopping native processes and docker containers.
  • Maybe we should relocate this trait to more upper hierarchy for implement stopping logics abstractly to all platforms.
  1. Add some libraries
  • anyhow・・・ for easier generic error handling
  • bollard・・・for docker client in rust
  • tokio・・・for asynchronous runtime (because the bollard is only supported tokio runtime currently)

Sorry for some huge changes. 🙏

Related issue(s)
If this PR is related to an existing issue, please link to it using the Fixes #issue_number or Closes #issue_number syntax.

Type of change
Please select one or multiple of the following options:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code cleanup (refactoring or improving code quality)
  • Documentation update (adding or updating documentation, updating README)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Additional information
Add any other information or screenshots about the pull request here.

@0kate 0kate force-pushed the feature/stop-docker-containers-for-linux branch from ff9b5f7 to bad27f5 Compare May 6, 2023 04:34
@0kate 0kate changed the title Feature/stop docker containers for linux Stop docker containers on linux May 6, 2023
@0kate 0kate marked this pull request as ready for review May 6, 2023 04:58
@0kate 0kate mentioned this pull request May 6, 2023
@jacobtread
Copy link
Contributor

I don't think that this commit really needs to introduce the anyhow crate from the looks of your changes all the errors already have some sort of mapping to std::io::Error and those that can't can just use the ErrorKind::Other from your usages it might be worth just mapping your errors into io errors instead of adding an extra crate

@jkfran
Copy link
Owner

jkfran commented May 10, 2023

Hi @0kate, thank you so much for the contribution! Would it be possible to eliminate all the unwrap()? I know some of them were already present, but we should aim to eliminate them from killport.

About anyhow, I agree with the previous comment. In our case, we're only dealing with std::io::Error and a few other error types that can be easily converted to std::io::Error. It might be simpler and more consistent to stick with std::io::Error.

@0kate
Copy link
Contributor Author

0kate commented May 12, 2023

@jacobtread @jkfran
Thank you for your confirming!
I understood about anyhow and only dealing std::io::Error. That's certainly right.

It's okay about eliminating unwrap() too!
I will add changes above 👍

@0kate 0kate force-pushed the feature/stop-docker-containers-for-linux branch from eca3fa8 to 25626a1 Compare May 13, 2023 07:24
@0kate
Copy link
Contributor Author

0kate commented May 13, 2023

@jkfran
Hello. I have eliminated unwrap and using error dealing with "anyhow" crate.
Please confirm that changes. 🙏

@jkfran jkfran force-pushed the feature/stop-docker-containers-for-linux branch from 3b7f2c9 to b556203 Compare July 26, 2023 20:19
@banool
Copy link

banool commented Sep 15, 2023

Hey I'd love to see this landed, any other changes needed?

@jkfran jkfran merged commit 6c77f65 into jkfran:main May 14, 2024
11 checks passed
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.

4 participants