-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Move output of stack rm to stdout #491
Conversation
Why should it go to stdout? |
Because it's not an error? See the following similar operations who send message to stdout: |
Codecov Report
@@ Coverage Diff @@
## master #491 +/- ##
======================================
Coverage 48.8% 48.8%
======================================
Files 199 199
Lines 16389 16389
======================================
Hits 7998 7998
Misses 7976 7976
Partials 415 415 |
Ok, makes sense to be consistent with the other remove commands. The e2e test needs to be fixed as well |
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.
LGTM with a squash
"Removing config bar_config1", | ||
"Removing network bar_network1\n", | ||
} | ||
assert.Equal(t, strings.Join(expectedList, "\n"), fakeCli.OutBuffer().String()) | ||
assert.Contains(t, fakeCli.ErrBuffer().String(), "Nothing found in stack: foo\n") |
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.
hmm, actually this seems weird. Why is it outputing both "nothing found in stack" and that it's removing things?
Not sure how this is passing. Something seems wrong
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.
Oh, nevermind, I see it now. There are two stacks, one is empty, the other is not.
Can you squash on merge? |
Signed-off-by: French Ben <[email protected]> Update for the test to capture the proper removal Signed-off-by: French Ben <[email protected]> Satisfy lint length limit Signed-off-by: French Ben <[email protected]> Updated e2e test Signed-off-by: French Ben <[email protected]>
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.
LGTM 🐮
- What I did
Updated the output of
docker stack rm
to be sent to stdout.Fixes #490
- How I did it
s/Err/Out
- How to verify it
With no errors, doing
docker stack rm nginx 1>stdout.log 2>stderr.log
should show all output instdout
- Description for the changelog
docker stack rm
now sends info messages to Stdout- A picture of a cute animal (not mandatory but encouraged)