Display deployment status for cf app command [v8] #3041
Conversation
Co-authored-by: Michael Chinigo <michael.chinigo@broadcom.com>
|
|
||
| It("returns the warnings and error", func() { | ||
| Expect(executeErr).To(MatchError("some-error")) | ||
| Expect(warnings).To(ConsistOf( |
There was a problem hiding this comment.
QUESTION
Are all of these warnings relevant to this scenario? If not, maybe the ContainElement matcher is more appropriate — otherwise this assertion will need to be updated any time any of the other, unrelated warnings are changed.
There was a problem hiding this comment.
Yes, it is because of when the test is happening (after all other function stubs are invoked) as we saw earlier.
There was a problem hiding this comment.
I get that these warnings are all returned in this scenario. But IMHO they're not all relevant to this scenario, and therefore should probably not be enumerated.
If some logic changes for one of the other warnings we'd need to rewrite this assertion to match. The test becomes more robust — and more expressive — by limiting the assertion to checking for just the presence of the single, relevant warning and not the exhaustive contents of the full array.
There was a problem hiding this comment.
I agree. We discussed this while working on these tests and moved it around to suit the process when we were pairing. If something changes for these warning, most of the tests in this will need to change.
| When("there is no active deployment", func() { | ||
| BeforeEach(func() { | ||
| summary = v7action.DetailedApplicationSummary{ | ||
| Deployment: resources.Deployment{ |
There was a problem hiding this comment.
QUESTION
Same as above:
- Does this represent an active
Deployment? What does that actually look like when it comes over the wire? - Is this even a valid state for the
DeploymentDTO?
There was a problem hiding this comment.
Same response as above. #3041 (comment)
| @@ -28,6 +28,12 @@ const ( | |||
| type DeploymentStatusReason string | |||
|
|
|||
| const ( | |||
| // DeploymentStatusReasonDeploying means the deployment is in state 'DEPLOYING' | |||
There was a problem hiding this comment.
SUGGESTION
These comments don't really add much information on top of the names. Just delete them?
There was a problem hiding this comment.
I'm ok with it. Just placed them for consistency sake.
There was a problem hiding this comment.
Would actually suggest deleting all the other ones, then. None of them add any information beyond what the identifier itself does.
Co-authored-by: Sam Gunaratne <385176+Samze@users.noreply.github.com>
| NewProcesses []Process `json:"new_processes,omitempty"` | ||
| Droplet Droplet `json:"droplet,omitempty"` | ||
| NewProcesses []Process `json:"new_processes,omitempty"` | ||
| Strategy constant.DeploymentStrategy `json:"strategy"` |
There was a problem hiding this comment.
thought/non-blocking Not strictly part of this PR, but I'm not sure its wise to have an enum here for the deployment strategies/states in the CLI. If the CLI is pointed at a newer version, say with canaries, then this will throw an error.
It's probably wiser to just allow arbitrary strings in the unmarshaling here and only use constants in the CLI when the CLI needs to understand the specific context of the Deployment.
There was a problem hiding this comment.
I concur @Samze. (Both in substance and that this shouldn't block a merge.)
There are probably lots of cases where some CLI command doesn't care about the value of this field, but it will blow regardless up at the deserialization step when we use this enum.
Zooming out even further — it would be lovely if we could use some machine-readable API descriptor to generate these DTOs rather than trying to keep them in sync manually. Do y'all publish, e.g., an OpenAPI spec anywhere that we could consume?
There was a problem hiding this comment.
Unfortunately there currently is no OpenAPI schema for Cloud Controller API - cloudfoundry/cloud_controller_ng#2192
Description of the Change
When the
cf appcommand is executed with strategy rolling, it displays the rolling deployment status(DEPLOYING or CANCELING) if one is in progress.Ex.
Rolling deployment is DEPLOYING