-
Notifications
You must be signed in to change notification settings - Fork 156
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
Carry over network config #195
Carry over network config #195
Conversation
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.
other than the 3 things above the code looks clean! Ill wait for your replies on these reviews :)
7707f2f
to
26be061
Compare
@dirtycajunrice Thanks for the review, I refactored the code and squashed my commits. Please have a look again 🙂 |
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.
Last thing above then you have my thumbs up. will need one more ok from tk or circa then good to go
Signed-off-by: julian <[email protected]>
26be061
to
01d3209
Compare
Wonderful. Youve got my approval. Still need one from TK or Circa. As an FYI, we might migrate this block somewhere else later on as we arent sure if it being in dockerclient is the right final resting place for the logic, but Its a great submission and we appreciate you helping out! |
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.
Appreciate the contribution!
Like @dirtycajunrice said, may find a better place for the logic to reside, but the core functionality looks great!
Fixes #193
I tested this fix in the following way: