-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add --network-mode option #674
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new Sequence diagram for container setup with network modesequenceDiagram
participant User
participant CLI
participant Model
participant Container
User->>CLI: run command with --network-mode
CLI->>Model: setup_container(args)
Note over Model: Check if subcommand is 'run'
Model->>Model: Add network mode to container args
Model->>Container: Create container with network settings
Container-->>User: Container running with specified network mode
Class diagram showing CLI argument changesclassDiagram
class ArgumentParser {
+add_argument()
}
class RunParser {
+ARGS: list
+network_mode: str
}
class Model {
+setup_container(args)
}
RunParser --|> ArgumentParser
Model ..> RunParser: uses
note for RunParser "Added --network-mode argument"
note for Model "Modified to handle network mode"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rhjostone - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fa591f4
to
9f83855
Compare
This option should be available for run and serve. Basically for any container operation. |
I am assuming this is related to --network=host and --network=private? |
1e71e79
to
c925324
Compare
Yeah, this should let you set any network driver that's supported in podman/docker: https://docs.podman.io/en/latest/markdown/podman-run.1.html#network-mode-net |
Thanks @rhjostone this is very good. Please squash your commits. git rebase -i origin |
c925324
to
5a392cd
Compare
Done! |
LGTM |
alright, lets do it. LGTM Thanks for checking @rhjostone |
Thanks @dougsland @rhatdan! |
5a392cd
to
ed4f322
Compare
Test wants the options to be alphabetically sorted in the man pages. |
ed4f322
to
59adf1a
Compare
@rhatdan Does the ordering in the man pages look correct now? |
Signed-off-by: Joshua Stone <[email protected]>
59adf1a
to
dc408f8
Compare
CI/CD not related to this change. Personally, I would love to have a documentation explaining the options and showing what happens if you change from one option to another. However, can be in a different patch. |
As he fixed the last milestone as @rhatdan requested and the patch looks good, moving forward. Thanks @rhjostone |
This PR should have not been merged @dougsland it didn't pass a single bats build |
Opening a revert PR. @rhjostone after the revert is complete, could you push this PR again? We need to then fix the tests so we can merge it when it's green. A lot of people are opening PRs right now and they are gonna fail unless we revert. |
Could you please share the error @ericcurtin ? To me the CI/CD is already broken for ages. |
This PR is reverted. |
The error is in the CI logs in this PR. @rhjostone can you re-push this PR, work through the tests to get them passing please. |
@ericcurtin It looks like the PR was reverted, then re-merged thanks to @rhatdan: Do I still have to re-push the PR? |
No, I took care of it. Thanks alot @rhjostone |
Good to know, thanks! |
Addresses #665
Summary by Sourcery
New Features: