Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

manifest_diff showing diff where top-level app fields incorrectly are being compared to web process-level fields #2243

Closed
elenasharma opened this issue Apr 22, 2021 · 3 comments

Comments

@elenasharma
Copy link
Contributor

Thanks for submitting an issue to cloud_controller_ng. We are always trying to improve! To help us, please fill out the following template.

Issue

The manifest_diff endpoint shows an incorrect diff when top-level application fields conflict with web process fields.

Context

When I create a manifest diff with a manifest like the following:

---
applications:
- name: test-app
  memory: 1024M
  processes:
  - type: web
    instances: 2
    memory: '256M'

I get an incorrect diff result:

{
   "diff": [
      {
         "op": "replace",
         "path": "/applications/0/memory",
         "was": "256M",
         "value": "1024M"
      }
   ]
}

The resulting diff has the top-level application memory value of "1024M" replacing the web process' memory value of "256M", even though the process memory has not been replaced.

Steps to Reproduce

  1. Push an application with a manifest in which there's a top-level application field (e.g. memory) that is the same as a web process field.
  2. Without altering the manifest file, use the space guid of the space that the above app is in to create a manifest diff e.g. cf curl /v3/spaces/<SPACE_GUID>/manifest_diff -X POST -H "Content-Type: application/x-yaml" -d @<PATH_TO_MANIFEST_FILE>
  3. Observe that in the resulting diff, the web process value appears to be replaced by the top-level application value.

Expected result

I would expect to see no diff, e.g.

{
   "diff": []
}

Current result

The current result is a diff with a "replace":

{
   "diff": [
      {
         "op": "replace",
         "path": "/applications/<PROCESS_INDEX>/<FIELD>",
         "was": "<PROCESS_VALUE>",
         "value": "<TOP_LEVEL_APPLICATION_VALUE"
      }
   ]
}
@sweinstein22 sweinstein22 added this to the manifest-diffs milestone May 6, 2021
tstannard pushed a commit that referenced this issue Jun 4, 2021
github issue #2243

Authored-by: Teal Stannard <[email protected]>
@elenasharma elenasharma changed the title manifest_diff showing diff where top-level app fields incorrectly replacing web process-level fields manifest_diff showing diff where top-level app fields incorrectly are being compared to web process-level fields Jun 7, 2021
@sweinstein22
Copy link
Contributor

After further discussion we've decided to ignore top-level memory and disk-quota keys, as they're not part of the supported, documented v3 manifest schema. Those keys will still have an impact when applying manifests or pushing applications in order to preserve backwards compatibility, however they will be ignored in visual app manifest diffs (the GET /v3/apps/:guid/manifest endpoint)

@bepotts
Copy link
Contributor

bepotts commented Jun 10, 2021

This issue has been addressed and merged into main. The merge can be viewed here.

@sethboyles
Copy link
Member

closing as this was fixed a while ago

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

No branches or pull requests

5 participants