Implement the /v3/apps/{guid}/manifest endpoint#4280
Conversation
uzabanov
force-pushed
the
issues/4258-implement-app-manifest-endpoint
branch
2 times, most recently
from
670d41e to
5224a8b
Compare
| }), nil | ||
| } | ||
|
|
||
| func (s StateCollector) getDroplet(ctx context.Context, authInfo authorization.Info, dropletGUID string) (repositories.DropletRecord, error) { |
There was a problem hiding this comment.
This function has the same signature of the droplet repo GetDroplet method and brings no value vs just inlining the droplet invocation.
However, not all apps have droplets. Imagine an app that has no package assigned, for example via cf create-app command. Getting the droplet for such an app would fail with not-found error thus causing the endpoint invocation fail. Maybe we should ignore the not-found error (which should probably make the function worth it)? And if the droplet is optional, than would returning a pointer to a droplet record make sense?
| func routeURL(route repositories.RouteRecord) string { | ||
| if route.Host != "" { | ||
| return fmt.Sprintf("%s.%s%s", route.Host, route.Domain.Name, route.Path) | ||
| } else { |
There was a problem hiding this comment.
We could spare the else:
func routeURL(route repositories.RouteRecord) string {
if route.Host != "" {
return fmt.Sprintf("%s.%s%s", route.Host, route.Domain.Name, route.Path)
}
return fmt.Sprintf("%s%s", route.Domain.Name, route.Path)
}
or even (to avoid the negation)
func routeURL(route repositories.RouteRecord) string {
if route.Host == "" {
return fmt.Sprintf("%s%s", route.Domain.Name, route.Path)
}
return fmt.Sprintf("%s.%s%s", route.Host, route.Domain.Name, route.Path)
}
| Expect(dropletGUID).To(Equal("droplet-guid")) | ||
| }) | ||
|
|
||
| It("returns empty droplet", func() { |
There was a problem hiding this comment.
when the app has no droplet, the droplet repo would return a not found error. You should adjust the default droplet repo stubbing to reflect that.
Then the Droplet field on the appState should be either nil or zero, depending on whether it is a pointer or struct
| }) | ||
|
|
||
| Describe("GET /v3/apps/{guid}/manifest", func() { | ||
| var dropletRecord repositories.DropletRecord |
There was a problem hiding this comment.
How about making dropletRecord local to the BeforeEach below (i.e. do not var it here)?
| Describe("GET /v3/apps/{guid}/manifest", func() { | ||
| var dropletRecord repositories.DropletRecord | ||
| BeforeEach(func() { | ||
| processRecord := repositories.ProcessRecord{ |
There was a problem hiding this comment.
This BeforeEach is doing quite a lot of initialisation on records. Do all those values make it to the manifest eventually? If not, we could get rid of the unused to minimise the unused boilerplate
uzabanov
force-pushed
the
issues/4258-implement-app-manifest-endpoint
branch
2 times, most recently
from
68bcdf6 to
f8ffd76
Compare
Implementing the app manifest endpoint includes calliing the `CollectState` method which gives as full information as possible for the `ManifestApplication` type (Routes,Processes,Services,AppRecord,Droplet). The following changes are made: * Calling `GetDroplet` in `Collect state` to encapsulate the build record in the `AppState` * Implementing `ToManifest` method to `AppState` type to map records to the `Manifest` types * It turned out that the routes repository returns only the `GUID` not the full `DomainRecord`, so the domainrepo is injected in the route repository and gets the full domain record for a route. Also unneccesary look ups for the domain are removed from the route and domain handler * Smoke test from the `cf create-app-manifest` command which calls the implemented manifest endpoint Closes #4258
uzabanov
force-pushed
the
issues/4258-implement-app-manifest-endpoint
branch
from
f8ffd76 to
317d8ae
Compare
| } | ||
|
|
||
| func toManifestProcesses(processes map[string]repositories.ProcessRecord) []payloads.ManifestApplicationProcess { | ||
| return slices.Collect(it.Right(it.Map2(maps.All(processes), func(i string, record repositories.ProcessRecord) (string, payloads.ManifestApplicationProcess) { |
There was a problem hiding this comment.
Wouldn't it be simpler to do
return slices.Collect(it.Map(maps.Values(processes), func(record repositories.ProcessRecord) payloads.ManifestApplicationProcess {...}))
Then line 41 could become
Type: record.Type,
After all it.Right(it.Map2(...)) just takes the value from the resulting map, largely ignoring the key.
| } | ||
|
|
||
| func toManifestServices(serviceBindings map[string]repositories.ServiceBindingRecord) []payloads.ManifestApplicationService { | ||
| return slices.Collect(it.Right(it.Map2(maps.All(serviceBindings), func(i string, record repositories.ServiceBindingRecord) (string, payloads.ManifestApplicationService) { |
georgethebeatle
deleted the
issues/4258-implement-app-manifest-endpoint
branch
3 participants
Is there a related GitHub Issue?
#4258
What is this change about?
Implementing the app manifest endpoint includes calliing the
CollectStatemethod which gives as fullinformation as possible for the
ManifestApplicationtype(Routes,Processes,Services,AppRecord,Droplet).