add API endpoints to Terminate a instances (cf restart-app-instance) #3351
Conversation
go.mod
Outdated
| @@ -51,6 +51,7 @@ require ( | |||
| github.com/containerd/log v0.1.0 // indirect | |||
| github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect | |||
| github.com/distribution/reference v0.6.0 // indirect | |||
| github.com/evanphx/json-patch v5.9.0+incompatible // indirect | |||
There was a problem hiding this comment.
Is this really needed? It is an indirect dependency so no under our direct control, but it looks weird
api/handlers/app_test.go
Outdated
| DesiredInstances: 1, | ||
| }, | ||
| }, nil) | ||
| req = createHttpRequest("DELETE", "/v3/apps/"+appGUID+"/processes/web/instances/0", nil) |
There was a problem hiding this comment.
It is a good practice to have the test call separated in a JustBeforeEach. This JustBeforeEach can be defined at the top level potentially parametrizing the number of the instance to be deleted. This number can be set in each When's respective BeforeEach. This way we will not have to repeat this call in every BeforeEach
api/handlers/app_test.go
Outdated
| @@ -1772,6 +1774,113 @@ var _ = Describe("App", func() { | |||
| }) | |||
| }) | |||
| }) | |||
|
|
|||
| Describe("DELETE /v3/apps/:guid/processes/:process/instances/:instance", func() { | |||
There was a problem hiding this comment.
Maybe a better structure for this describe would be
Describe("DELETE /v3/apps/:guid/processes/:process/instances/:instance", func() {
It("returns process not found error", func() {})
When("the app does not exist", func() {
It("returns an app not found error", func() {})
})
When("the app has a process", func() {
It("returns an instance not found error", func() {})
When("the process has an instance") {
It("restarts the instance", func() {})
})
When("the app has more than one process", func() {
It("restarts the instance", func() {})
})
})
})
The idea is that each when builds on top of its parent. This way a lot of repetition can be avoided
api/handlers/process_test.go
Outdated
| @@ -454,4 +460,79 @@ var _ = Describe("Process", func() { | |||
| }) | |||
| }) | |||
| }) | |||
|
|
|||
| Describe("DELETE /v3/processes/:guid/instances/:instance", func() { | |||
There was a problem hiding this comment.
We could restructure this Describe in the spirit of the apps handler one
- Start with no instance an no process
- When there is an instance
- When there is a process
api/handlers/process_test.go
Outdated
|
|
||
| }) | ||
|
|
||
| JustBeforeEach(func() { |
There was a problem hiding this comment.
We could pull the JustBeforeEach up and parametrize the instance number to avoid repeating essentially the same JustBeforeEach in every When
|
|
||
| }) | ||
| Describe("DeletePod", func() { | ||
| When("the user is not a a SpaceDeveloper", func() { |
There was a problem hiding this comment.
This When can be nested in the describe as it could be looked at as the default case
| When("the user is not a a SpaceDeveloper", func() { | ||
|
|
||
| It("fails to delete the pod", func() { | ||
| Expect(podRepo.DeletePod(ctx, authInfo, *app, "2", "web")).To(HaveOccurred()) |
There was a problem hiding this comment.
It would be more idiomatic to extract the DeletePod invocation to a global JustBeforeEach, parametrize the arguments and var the error in the Describe, so that we can check it in the inidividual Its. This way we will be calling DeletePod in just one place in the test.
api/handlers/process.go
Outdated
| ) *Process { | ||
| return &Process{ | ||
| serverURL: serverURL, | ||
| processRepo: processRepo, | ||
| processStats: processStatsFetcher, | ||
| requestValidator: requestValidator, | ||
| appRepo: appRepo, |
There was a problem hiding this comment.
Injecting an app repo to the process handler looks weird to me. We only need the app record so that we know the app revision, so maybe we could extend the process repo by adding a GetAppRevisionForProcess method. It would not duplicate a lot of the app repo code since it is simply a k8sClient.Get call. On the other hand the app repo contains a lot of methods that the process repo should not be able to see. In order to do that we will need to modify the DeletePod signature to not take an AppRecord but appGUID, appSpaceGUID, and appRevision as three arguments, which is not a great thing as well, but to me it is more acceptable than giving the app repo to the process handler.
|
Hey @marsteg thanks for your PR, I have left some inline comments. In technical terms your PR looks good and the comments address some stylistic improvements. In more abstract terms adding more behaviour to the PodRepo is something we are trying to avoid, since it is breaking Korifi's abstraction and assuming that apps are rendered into pods in a very specific way that is implemented by the default statefulset runner. This will make it hard to potentially swap the current implementation with other ones - like a Deployment runner or a multi cluster runner. Having said that we have not yet thought of a way to fix this abstraction leak, so I cannot propose a better approach for the time being. Can you elaborate on what you need that feature for and what is the priority of the use case? |
Is there a related GitHub Issue?
#3350
What is this change about?
Adding cf restart-app-instance API endpoints
Does this PR introduce a breaking change?
Not that I know of.
Acceptance Steps
in the cf cli, the command:
cf restart-app-instance test-app 3 --process web -v
works
Tag your pair, your PM, and/or team
@danail-branekov @georgethebeatle