Validate -i value for ssh command is an existing app instance

[davwards]

What Need Does It Address?

Without this change, passing a -i value that is equal to or greater than
the total number of instances the application has results in a cryptic
error message about the ssl handshake, which can lead users down a false
trail.

Possible Drawbacks

To keep the validation checks for the -i flag together, I moved the not-negative validation check after the creation of cmd.opts and cmd.appReq. No tests were upset by this, but it could be the case that due to factors I didn't catch--if populating cmd.appReq is expensive, for instance--this could result in slower feedback when you enter a negative value for -i.

Also, I didn't add any i18n translations for the new message. Note: When trying to run bin/i18n-checkup, I observed that the schema of the translation .json files was completely changed. On Slack, @anand indicated that this was probably due to the fact that the team is using a particular version of i18n4go that is different than the one I got with go get, and that I should not make any changes to the translation files and submit anyway.

Why Should This Be In Core?

It improves the messaging of a core feature, as opposed to adding new functionality.

Description of the Change

In addition to checking that the -i value is non-negative, fetch the InstanceCount of the specified application and ensure the -i value is less than that. If not, fail with a descriptive message.

Applicable Issues

#1130

[cfdreddbot]

Hey davwards!

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

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/144799125

The labels on this github issue will be updated when the story is started.

[n4wei]

@davwards When I have one running instance of the app "my-app", the following no longer works:

$ cf ssh my-app -i 0
FAILED
Incorrect Usage: The specified application instance does not exist

NAME:
   ssh - SSH to an application container instance
...

The Requirements method is meant for setup so that the Execute method can perform the core logic of the ssh command. What you have could work, if you did a cmd.appReq.Execute() before performing the check. This will make the API call to get the application object. Without it, calling cmd.appReq.GetApplication() returns an empty application model with instance count defaulting to 0, thus the scenario above.

However, the calling function that runs Requirements and Execute is already responsible for running cmd.appReq.Execute(). It would be much cleaner to move this check in the Execute method. Please make this change and let us know when you're ready for us to review.

[davwards]

I see! That suggests that the way I simulated the situation in ssh_test wasn't true to life; how would I do that properly?

[n4wei]

@davwards At the layer that was being unit-ish tested, you were doing the right thing. The correct way to expose this bug would be through integration tests. You can add a test for this here, and you can use this as an example.

[davwards]

Hi all; I'm getting pulled off the Labs beach, so my available time to work on this is diminishing. Getting the bosh-light integration test environment set up is proving pretty difficult. All this to say, I don't expect to be able to deliver an integration-test-driven fix soon.

@nickwei84, I also feel like my understanding of the test setup is incomplete; it seems odd to me that the unit test I wrote could be correct, given that the implementation was completely broken. Is there not a way to simulate the life-like behavior of cmd.appReq.Execute() and cmd.appReq.GetApplication()? (In particular, the fact that GetApplication() returns an empty model until Execute() has been called.)

[n4wei]

@davwards I'd recommend getting a PWS account and test against that.
The unit tests for the Requirements() and Execute() methods intrinsically have different assumptions. The calling function in cf/cmd/cmd.go should tell you what these assumptions are link to code chunk. Following the order that things are called, stubbing out GetApplications() in the Execute() method would have been the right thing to do.

[dkoper]

@davwards Please let us know if you think you won't be able to get to this any time soon and would like us to finish the job.

[davwards]

@dkoper Indeed, feel free to wrap this up; I don't want it to get stuck in limbo waiting on me.

[stuart-pollock]

resolved the issues which arose in an initial review