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

**CLI** user can see a **diff** when **pushing** that respects **arguments and flags** #2196

Closed
sethboyles opened this issue Apr 19, 2021 · 4 comments

Comments

@sethboyles
Copy link
Member

Acceptance Criteria

App Name

Given a manifest file "manifest.yml" with multiple apps in it: app1, app2, and app3
When I cf push app1 -f manifest.yml, specifying a single app to push
Then I should only see a diff for app1
And it should show all the fields for app1 in manifest.yml (not the other apps in the manifest)


Push Flags

Given a manifest file "manifest.yml" with one or more apps in it
When I cf push -f manifest.yml with one or more other flag overrides (e.g. -m, -k, etc)
Then the diff should take the flag overrides into account

Dev Notes

This might already be implemented, it's just difficult to tell given the currently in-flight bug that we're working on (https://www.pivotaltracker.com/story/show/177552249).
This story will be to verify the desired behavior exists.

@sweinstein22 sweinstein22 added this to the manifest-diffs milestone May 6, 2021
@tjvman
Copy link
Member

tjvman commented May 10, 2021

We did some manual testing on this issue. Based on the results we got, it seems that the desired behavior is already present.

Setup

Testing done using a GCP BOSH-lite-based CF running CF-deployment 16.14.0 and a locally-built CAPI running cloud_controller_ng at revision f26cf679955c.

We used pora as our test application. The manifests we used are at the end of this comment.

Results

  • Push new app from multi-app manifest
    Result: Works as intended - only displays diff for the new app
  • Update app using a single-app manifest with the -k, -i, -m flags specified.
    Result: Works as intended - the diff shows the values from the command-line flags instead of the values in the manifest.
  • Update one app from multi-app manifest with -k, -i, -m flags specified.
    Result: Works as intended - the diff shows the values from the command-line flags instead of the values in the manifest.
  • Push changed app from multi-app manifest
    Result: Works as intended (only displays diff for the specified app)
  • Push unchanged app from multi-app manifest when other apps in the manifest have been changed
    Result: Works as intended - shows no diff

Test Manifests

Single-app manifest:

---
applications:
- name: single
  buildpacks:
  - go_buildpack
  env:
    GOPACKAGENAME: cora
    VAR: foobar
  processes:
  - type: web
    instances: 2
    disk_quota: 1024M
  - type: worker
    instances: 1
    memory: 1024M
    disk_quota: 1024M
    health-check-type: port
  sidecars:
  - name: sleep_agent
    process_types: [ 'web', 'worker' ]
    command: sleep 1000

Multi-app manifest:

---
applications:
- name: multi1
  buildpacks:
  - go_buildpack
  env:
    GOPACKAGENAME: cora
    VAR: foobar
  processes:
  - type: web
    instances: 2
    disk_quota: 1024M
  - type: worker
    instances: 1
    memory: 1024M
    disk_quota: 1024M
    health-check-type: port
  sidecars:
  - name: sleep_agent
    process_types: [ 'web', 'worker' ]
    command: sleep 1000

- name: multi2
  buildpacks:
  - go_buildpack
  env:
    GOPACKAGENAME: cora
    VAR: foobar
  processes:
  - type: web
    instances: 1
    disk_quota: 1000M
  - type: worker
    disk_quota: 512M
    health-check-type: port
  sidecars:
  - name: sleep_agent
    process_types: [ 'web', 'worker' ]
    command: sleep 1000

@elenasharma
Copy link
Contributor

elenasharma commented May 13, 2021

For acceptance:

App name

The diff didn't show up as expected.

my steps:

  • used the multiple app manifest described above and called it "multi_manifest.yml`

  • ran cf push -f multi_manifest.yml

  • modified the manifest file to remove the env key from the first app, multi1:

    ---
    applications:
    - name: multi1
      buildpacks:
      - go_buildpack
      processes:
      - type: web
        instances: 2
        disk_quota: 1024M
      - type: worker
        instances: 1
        memory: 1024M
        disk_quota: 1024M
        health-check-type: port
      sidecars:
      - name: sleep_agent
        process_types: [ 'web', 'worker' ]
        command: sleep 1000
    
    - name: multi2
      buildpacks:
      - go_buildpack
      env:
        GOPACKAGENAME: cora
        VAR: foobar
      processes:
      - type: web
        instances: 1
        disk_quota: 1000M
      - type: worker
        disk_quota: 512M
        health-check-type: port
      sidecars:
      - name: sleep_agent
        process_types: [ 'web', 'worker' ]
        command: sleep 1000
    
  • ran cf push multi1 -f multi_manifest.yml

  • got this result:

cf push multi1 -f multi_manifest.yml
Pushing app multi1 to org org / space outer as admin...
Applying manifest file multi_manifest.yml...

Updating with these attributes...
  applications:
  - name: multi1
    processes:
    - disk_quota: 1024M
      instances: 2
      type: web
    - disk_quota: 1024M
      health-check-type: port
      instances: 1
      memory: 1024M
      type: worker
    default-route: true
    buildpacks:
    - go_buildpack
    sidecars:
    - command: sleep 1000
      name: sleep_agent
      process_types:
      - web
      - worker

While these manifest properties are technically correct, and only the properties for multi1 are showing up, what I am not seeing - which I expected to see - is the diff formatted and red for where I removed env e.g.

-   env:
-     GOPACKAGENAME: cora
-     VAR: foobar

Push Flags

Agree that this looked good. I pushed the app with various flag overrides (-i, -k, -m), and the diff used the value(s) from the flag(s).

@tjvman
Copy link
Member

tjvman commented May 14, 2021

I think this behavior is correct. Currently, the diffing logic returns an "effective" diff (i.e. it only marks changes that will actually cause a change to the app), not a "literal" diff (what you'd get if you ran the old & new manifests through diff or something similar).

In the above case, removing the env vars from the manifest doesn't actually change the app (because manifest changes are additive) - running cf env multi1 after the second push will still include GOPACKAGENAME and VAR in the output. Since the change to the manifest doesn't have an effect, the diffing logic doesn't mark it.

@elenasharma
Copy link
Contributor

Okay that makes sense to me. Thanks for explaining that behavior, and why it's expected. The rest of the issue looks like the expected behavior was working!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants