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

Add --prev-app to delete command #511

Merged
merged 6 commits into from
May 12, 2022
Merged

Conversation

neil-hickey
Copy link
Contributor

@neil-hickey neil-hickey commented May 9, 2022

  • This PR assumes the user wants to see the changeset diff on the cli
    also when providing a --prev-app. Hence the use of the Factory twice.
  • The case of both existing is interesting, for now it ignores the
    --prev-app existing if --app exists
  • Nothing inside recorded_app.go needs to change with this pattern

Signed-off-by: Neil Hickey nhickey@vmware.com

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?


Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


- This PR assumes the user wants to see the changeset diff on the cli
  also when providing a --prev-app. Hence the use of the Factory twice.
- The case of both existing is interesting, for now it ignores the
  --prev-app existing if --app exists
- Nothing inside recorded_app.go needs to change with this pattern

Signed-off-by: Neil Hickey <nhickey@vmware.com>
@neil-hickey
Copy link
Contributor Author

neil-hickey commented May 9, 2022

Some things to note:

  1. I chose to use the Factory twice - allows most of the code to remain as is in delete.go and recorded_app.go doesn't need to change at all.
  2. This may introduce other problems I am not fully thinking through right now, I wanted to check with @praveenrewar and others to see what you all thought.
  3. I wanted the user to see the diff and hit yes etc even for a --prev-app app. So this factory request for each app made most sense in that context as it shows this diff and confirmation already

Interesting case:
App1 exists, App2 exists

kapp delete -a App1 --prev-app App2

What would you expect the behaviour to me? Given that App2 should have been migrated and not exist at the same time, I expect App1 to be deleted and App2 to be ignored.

Any UX opinions on this case?

Signed-off-by: Neil Hickey <nhickey@vmware.com>
pkg/kapp/cmd/app/delete.go Outdated Show resolved Hide resolved
pkg/kapp/cmd/app/migration_flags.go Outdated Show resolved Hide resolved
pkg/kapp/cmd/app/migration_flags.go Outdated Show resolved Hide resolved
@praveenrewar
Copy link
Member

I chose to use the Factory twice - allows most of the code to remain as is in delete.go and recorded_app.go doesn't need to change at all.

I had something similar in my mind, so I am actually happy with this approach.

App1 exists, App2 exists
kapp delete -a App1 --prev-app App2
What would you expect the behaviour to me?

Given that App2 should have been migrated and not exist at the same time, I expect App1 to be deleted and App2 to be ignored.

I totally agree. We should just ignore App2 in this case as this scenario shouldn't have happened.

Co-authored-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com>
Signed-off-by: Neil Hickey <nhickey@vmware.com>
Signed-off-by: Neil Hickey <nhickey@vmware.com>
Signed-off-by: Neil Hickey <nhickey@vmware.com>
@neil-hickey neil-hickey marked this pull request as ready for review May 10, 2022 18:13
Signed-off-by: Neil Hickey <nhickey@vmware.com>
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@100mik 100mik left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@sethiyash sethiyash left a comment

Choose a reason for hiding this comment

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

Looks Good.

@praveenrewar praveenrewar merged commit 4a6a3a6 into develop May 12, 2022
@neil-hickey neil-hickey deleted the delete-prev-app-kc-376 branch May 12, 2022 16:45
@renuy
Copy link
Contributor

renuy commented May 31, 2022

Available as part v0.48.0

@github-actions github-actions bot added the carvel triage This issue has not yet been reviewed for validity label May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel triage This issue has not yet been reviewed for validity cla-not-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants