Skip to content

Fix resource modelling inconsistencies across apps/instances/deployments (#49) - #58

Open
DLondonoD wants to merge 2 commits into
camaraproject:mainfrom
DLondonoD:fix/issue-49
Open

Fix resource modelling inconsistencies across apps/instances/deployments (#49)#58
DLondonoD wants to merge 2 commits into
camaraproject:mainfrom
DLondonoD:fix/issue-49

Conversation

@DLondonoD

Copy link
Copy Markdown
Contributor

What type of PR is this?

correction

What this PR does / why we need it:

Fixes five resource-modelling inconsistencies across apps/instances/deployments:

  • Path naming: renames /appinstances to /app-instances, matching the
    hyphenated kebab-case naming used by every other multi-word path
    (/deployments, /clusters, /edge-cloud-zones).
  • Single-resource GET: adds GET /app-instances/{appInstanceId}
    (getAppInstance) and GET /deployments/{appDeploymentId}
    (getAppDeployment), for consistency with /apps' getApp. The former
    list operations are renamed to getAppInstances/getAppDeployments to
    free up the singular operationId for the new by-id GETs.
  • Location header consistency: fixes createAppInstance's 202
    Location header, which described the URI as pointing at "the newly
    created application" (copy-paste from /apps) and had no working GET
    target — it now correctly describes the application instance and
    references the new getAppInstance endpoint. Adds an equivalent
    Location header to createAppDeployment's 202 response, aligning it
    with createAppInstance's pattern and pointing at the new
    getAppDeployment endpoint.
  • Request/response schema split: splits AppManifest into a
    request-only shape (no appId, used by submitApp) and a new
    AppManifestInfo response shape (AppManifest allOf + required
    appId), used by both getApps' list items and getApp.
  • getApp/getApps shape alignment: getApp's response is unwrapped
    to return AppManifestInfo bare, matching getApps' item shape instead
    of the previous { appManifest: AppManifest } wrapper.

Additionally, now that getAppInstance/getAppDeployment exist as
single-resource GET-by-id operations, removes the now-redundant
appInstanceId/appDeploymentId query filters from
getAppInstances/getAppDeployments respectively — filtering a list by
the exact id it's supposed to enumerate is redundant with calling the
by-id endpoint directly, and this also aligns both list operations with
getApps, which has no analogous id-in-query filter.

Test feature files were added/updated accordingly (getAppInstance.feature,
getAppDeployment.feature new; getAppInstances.feature,
getAppDeployments.feature, getApp.feature, getApps.feature,
createAppInstance.feature, deleteAppInstance.feature updated).

Which issue(s) this PR fixes:

Fixes #49

Special notes for reviewers:

This is a breaking change to path/operationId naming
(/appinstances/app-instances, getAppInstancegetAppInstances,
getAppDeployments' query params), acceptable per the issue given the API
is still pre-1.0/rc. Reviewers should double check the AppManifest/
AppManifestInfo split doesn't affect any other operation still relying
on the original combined schema.

Changelog input

release-note Rename /appinstances to /app-instances;

Additional documentation

This section can be blank.

docs

…nts (camaraproject#49)

Renames /appinstances to /app-instances to match the hyphenated
kebab-case naming used by every other multi-word path (/deployments,
/clusters, /edge-cloud-zones).

Adds single-resource GET-by-id for both application instances
(GET /app-instances/{appInstanceId}, operationId getAppInstance) and
deployments (GET /deployments/{appDeploymentId}, operationId
getAppDeployment), for consistency with /apps' getApp. The former
list operations are renamed to getAppInstances/getAppDeployments to
free up the singular operationId for the new by-id GETs.

Fixes createAppInstance's 202 Location header, which described the
URI as pointing at "the newly created application" (copy-paste from
/apps) and had no working GET target. It now correctly describes the
application instance and references the new getAppInstance endpoint.
Adds an equivalent Location header to createAppDeployment's 202
response, aligning it with createAppInstance's pattern and pointing
at the new getAppDeployment endpoint.

Splits AppManifest into a request-only shape (no appId, used by
submitApp) and a new AppManifestInfo response shape (AppManifest
allOf + required appId), used by both getApps' list items and getApp.
getApp's response is also unwrapped to return AppManifestInfo bare,
matching getApps' item shape instead of the previous
{ appManifest: AppManifest } wrapper - this also matches what the
existing getApp/getApps test feature files already expected.

Fixes camaraproject#49
)

Now that getAppInstance (GET /app-instances/{appInstanceId}) and
getAppDeployment (GET /deployments/{appDeploymentId}) exist as
single-resource GET-by-id operations, filtering the list operations
(getAppInstances, getAppDeployments) by that same id via query
parameter is redundant with just calling the by-id endpoint directly.
Removes appInstanceId from getAppInstances' query parameters and
appDeploymentId from getAppDeployments', aligning both with getApps
(which has no analogous id-in-query filter either).

Updates the corresponding test feature files, removing the
now-invalid filtered-by-id scenarios.

@JoseMConde JoseMConde left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resource modelling inconsistencies across apps/instances/deployments

2 participants