Fix to the delete-domain to fail early if domain is shared

[SrinivasChilveri]

delete-domain command is not failing early if given name is a shared
domain and delete-shared-domain is not failing early if given name is
owned domain.

Modified the delete-domain and delete-shared-domain commands to check
the domain type and fail early based on domain type.

$ cf domains

Getting domains in org abc as admin...
name status
10.244.0.34.xip.io shared
abc.shared.com shared
abc.private.domain owned

Before Fix :
delete-domain abc.shared.com -f
Deleting domain abc.shared.com as admin...
FAILED
Error deleting domain abc.shared.com
Server error, status code: 404, error code: 130002, message: The domain could not be found: c42e00e4-0c72-4ba4-ba1c-cb4b59ff1bc0

$ cf delete-shared-domain abc.private.domain -f
Deleting domain abc.private.domain as admin...
FAILED
Error deleting domain abc.private.domain
Server error, status code: 404, error code: 130002, message: The domain could not be found: 51a6d739-5134-4dfd-8ed0-6e1d38c5a2e0

After Fix :
$ ./out/cf delete-domain abc.shared.com
domain abc.shared.com is not an owned domain

$ ./out/cf delete-shared-domain abc.private.domain
Deleting domain abc.private.domain as admin...
domain abc.private.domain is not a shared domain

[#68736518]

[cfdreddbot]

Hey SrinivasChilveri!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/103895494.

[SrinivasChilveri]

Hi @camelpunch ,

I have fixed all your comments.
Any suggestions,comments are welcome.

Thanks & Regards,
SriniCH

[camelpunch]

The BeforeEach() sets the domain to 'Shared: true', so this context should be something like, "when the domain is shared".

[SrinivasChilveri]

Hi @camelpunch ,

I have fixed all your comments.
Any suggestions,comments are welcome.

Thanks & Regards,
SriniCH

[camelpunch]

@goehmen how does the language sit with you? Do both "X is not an owned domain" and "X is not a shared domain" make sense in context?

[goehmen]

Yes they do.

Greg Oehmen
Cloud Foundry Product Manager
415.205.6596

On Wed, Sep 23, 2015 at 7:44 AM, Andrew Bruce notifications@github.com
wrote:

@goehmen https://github.com/goehmen how does the language sit with you?
Do both "X is not an owned domain" and "X is not a shared domain" make
sense in context?

—
Reply to this email directly or view it on GitHub
#596 (comment).

[camelpunch]

@simonleung8 @goehmen @squeedee

I'd expect this change to result in a nonzero exit code, but:

cli$ out/cf domains
Getting domains in org me as admin...
name                 status
10.244.0.34.xip.io   shared
cli$ out/cf delete-domain asdfasdf
OK
Domain asdfasdf not found
cli$ echo $?
0

Should this really be a 1 exit?

[simonleung8]

Looks like the default is a fail, so exit 1 seems appropriate
https://github.com/cloudfoundry/cli/blob/master/cf/commands/domain/delete_domain.go#L72

@SrinivasChilveri can you please make it a cmd.ui.Failed() instead of cmd.ui.Say()?

[SrinivasChilveri]

Hi @camelpunch @simonleung8 , I think you mean to change the below lines

case *errors.ModelNotFoundError:
cmd.ui.Ok()
cmd.ui.Warn(apiErr.Error())
to
case *errors.ModelNotFoundError:
cmd.ui.Failed(apiErr.Error())

both in delete-domain & delete-shared domain commands? am i correct?

Thanks & Regards,
SriniCH

[simonleung8]

Sorry I didn't make it clear. I meant changing the modified lines from Say() to Failed(). Since basically what happens here is that the domain exists, but the user is issuing a wrong command, it should exit non-zero.

    case nil:
        if domain.Shared {
            cmd.ui.Say(T("domain {{.DomainName}} is not an owned domain",
                map[string]interface{}{
                    "DomainName": domainName}))
            return
        }

[SrinivasChilveri]

Hi @camelpunch @simonleung8,
changes are done as suggested.

Thanks & Regards,
SriniCH

[camelpunch]

@SrinivasChilveri sorry for the back-forth, but we could really do with test coverage for that last change. For example, a test fails when I change the default case to Say, but not for the nil case that you've added.

[SrinivasChilveri]

HI @camelpunch ,Thanks a lot for your review. changes are done.

Thanks&Regards,
SriniCH