Address bug #1733920, don't tread model NotFound as an error #8125
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
axw
approved these changes
Nov 23, 2017
cmd/juju/model/destroy.go
Outdated
| // No need to give the user a warning that the model they asked | ||
| // to destroy is no longer there. | ||
| if errors.IsNotFound(status[0].Error) || params.IsCodeNotFound(status[0].Error) { | ||
| ctx.Infof("Model destroyed.") |
There was a problem hiding this comment.
If we're adding logging for this one, we should probably do it when checking the immedate error result of ModelStatus, too.
|
where would that be?
I only added this because it did seem like we left you hanging after giving
several "waiting for destruction" and then the command just returns without
actually indicating success or failure. And Not found in this case sure
seems to be success :)
I'm happy to move that message elsewhere but I'm not sure I understand your
request.
John
=:->
…On Nov 24, 2017 03:38, "Andrew Wilkins" ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In cmd/juju/model/destroy.go
<#8125 (comment)>:
> @@ -338,9 +338,13 @@ func newTimedModelStatus(ctx *cmd.Context, api DestroyModelAPI, tag names.ModelT
return nil
}
if status[0].Error != nil {
- // This most likely occurred because a model was
- // destroyed half-way through the call.
- ctx.Infof("Could not get the model status from the API: %v.", status[0].Error)
+ // No need to give the user a warning that the model they asked
+ // to destroy is no longer there.
+ if errors.IsNotFound(status[0].Error) || params.IsCodeNotFound(status[0].Error) {
+ ctx.Infof("Model destroyed.")
If we're adding logging for this one, we should probably do it when
checking the immedate error result of ModelStatus, too.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#8125 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAMYfV_NUNMMc3JFuJ8Ehc5O0V9gAqbiks5s5gIDgaJpZM4Qo2IQ>
.
|
Instead of just updating the error of the single response, merge the errors which was already handling when it was getting ErrNotFound.
anastasiamac
approved these changes
Nov 26, 2017
|
|
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
When destroying a model, we should expect it to eventually go away.
Don't treat NotFound as a real error.
QA steps
It used to give
Documentation changes
None
Bug reference
lp:1733920