Add client support for Swarm Logs and Swarm Config #589
Conversation
|
|
|
@galvesribeiro let me know I you need some unit tests against these changes. Thanks. |
|
Hello! Thanks for the contribution.
Can you please add some basic coverage to it? Thank you! |
There was a problem hiding this comment.
Thank you for submitting this pull request. I have reviewed the code and overall it looks good. However, as @galvesribeiro mentioned, tests are missing.
| using Models; | ||
| using System.IO; | ||
|
|
||
| internal class SwarmOperations : ISwarmOperations |
There was a problem hiding this comment.
It would be kind if we could remove all the (unnecessary) whitespace changes.
There was a problem hiding this comment.
Do you mean lineendings? I'm having trouble with this as it seems to me the repo has a mix of lineendings already? I ran git ls-files --eol and see both CRLF and LF.
My attempts at normalizing have resulted in even more files changed.
There was a problem hiding this comment.
I have similar issues with this repo all the time. It just makes it super hard to review SwarmOperations and ISwarmOperationsTests. Probably, it is necessary to encode and normalize the files all at once.
Co-authored-by: Andre Hofmeister <[email protected]>
Co-authored-by: Andre Hofmeister <[email protected]>
ACoderLife
force-pushed
the
master-plus-models-update
branch
4 times, most recently
from
227bfd5 to
6a30878
Compare
add logging test
fix naming convention
ACoderLife
force-pushed
the
master-plus-models-update
branch
from
6a30878 to
7c0e19e
Compare
| using Models; | ||
| using System.IO; | ||
|
|
||
| internal class SwarmOperations : ISwarmOperations |
There was a problem hiding this comment.
I have similar issues with this repo all the time. It just makes it super hard to review SwarmOperations and ISwarmOperationsTests. Probably, it is necessary to encode and normalize the files all at once.
|
|
||
| namespace Docker.DotNet | ||
| { | ||
| internal class ConfigsOperations : IConfigsOperations |
There was a problem hiding this comment.
Don't we need tests for this class too?
There was a problem hiding this comment.
Yes, I'll add them.
There was a problem hiding this comment.
Added: Create, List, Inspect and Remove tests.
Co-authored-by: Andre Hofmeister <[email protected]>
Co-authored-by: Andre Hofmeister <[email protected]>
Co-authored-by: Andre Hofmeister <[email protected]>
|
@galvesribeiro Did you want me to make any other clean up? |
|
Thanks for the PR! I would rename swarm-related objects as suggested and also fix the test. Everything else LGTM. |
|
@galvesribeiro renaming based on suggestions should be complete. Fixed test, @HofmeisterAn can you retry the CI build? Cheers. |
|
All good! Thank you @ACoderLife. Will cut a new build soon. |
This pull request finishes off work started by @The-TT-Hacker here:
#472
It could also close this:
#387
If you are happy with the API being client.Config and not client.Swarm.Config
and also surfaces the service logs.