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 flag to skip checking resources when fully delete app #594

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

firgavin
Copy link

@firgavin firgavin commented Aug 30, 2022

What this PR does / why we need it:

Add flag to skip checking resources when fully deletes app.

Which issue(s) this PR fixes:

Fixes #577

Does this PR introduce a user-facing change?

Add `--dangerous-disable-checking-app-deletion` flag to  disable checking resources when fully deletes app #594

Additional Notes for your reviewer:

  • Relevant carvel.dev docs will be added as soon as possible
  • I'm not sure about the impact of remaining resources on test envrionment, so I didn't write e2e test for this flag
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.:


@vmwclabot
Copy link

@firgavin, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@firgavin, we have received your signed contributor license agreement. It will be reviewed by VMware shortly. Another comment will be added to the pull request to notify you when the merge can proceed.

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.

Hey @firgavin! Thanks a lot for working on this :)
I have added a few comments. Also , I think we can add an e2e test for this.

pkg/kapp/cmd/app/delete.go Outdated Show resolved Hide resolved
pkg/kapp/cmd/app/delete_flags.go Show resolved Hide resolved
pkg/kapp/cmd/app/delete_flags.go Outdated Show resolved Hide resolved
pkg/kapp/cmd/app/delete_flags.go Outdated Show resolved Hide resolved
pkg/kapp/app/interfaces.go Outdated Show resolved Hide resolved
pkg/kapp/app/labeled_app.go Outdated Show resolved Hide resolved
pkg/kapp/cmd/app/delete_flags.go Outdated Show resolved Hide resolved
pkg/kapp/app/labeled_app.go Outdated Show resolved Hide resolved
@vmwclabot
Copy link

@firgavin, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

Please provide the name of your employer and full address.

@praveenrewar
Copy link
Member

Hey @firgavin! Were you able to sign the cla again with the relevant information? Do let me know if you would need any help :)

@vmwclabot
Copy link

@firgavin, we have received your signed contributor license agreement. It will be reviewed by VMware shortly. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

@firgavin, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

Please provide the name of the organization you are affiliated with or indicate none.

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.

Changes look good to me (we could probably add an e2e test although we can also do it as part of another PR as well) but it seems it's really hard to please the CLA bot. @firgavin Do let me know if there's anyway I can help or if there's any feedback you would like to share about the signing process.

@firgavin
Copy link
Author

Changes look good to me (we could probably add an e2e test although we can also do it as part of another PR as well) but it seems it's really hard to please the CLA bot. @firgavin Do let me know if there's anyway I can help or if there's any feedback you would like to share about the signing process.

Hi @praveenrewar - I have signed the Individual CLA, and there’s a warning message that says I should provide the name of the organization or indicate none. But where to provide?😂 Any suggestion? Thanks.

@praveenrewar
Copy link
Member

Hi @firgavin! Thanks for sharing the details, I have asked for help from folks internally and will let you know once I have an update. Thanks for being patient :)

@praveenrewar
Copy link
Member

Hi @firgavin, In the CLA form, there should be a field “Organization” (probably at the very bottom). Let me know if you are not able to find it or face any issue.

@firgavin
Copy link
Author

Hi @praveenrewar - Thanks for your feedback! Somehow I cannot find this field...Not sure if I am missing something. Here’s the screenshot:
image

@praveenrewar
Copy link
Member

Thanks for sharing the screenshot @firgavin. I have shared it with the team, hoping to get some feedback soon :)

@praveenrewar
Copy link
Member

Hi @firgavin can you please try to sign the CLA again, the warning should be gone now.

@vmwclabot
Copy link

@firgavin, we have received your signed contributor license agreement. It will be reviewed by VMware shortly. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

@firgavin, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

VMware is not able to accept your contribution at this time.

@vmwclabot
Copy link

@firgavin, we have received your signed contributor license agreement. It will be reviewed by VMware shortly. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

@firgavin, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

VMware is not able to accept your contribution at this time.

@praveenrewar
Copy link
Member

Hi @firgavin Can you please try signing the CLA again now. Apparently in some cases even individual contributors are required to provide the organisation that they are affiliated with, but the form didn't had the option to provide that information. It should be fixed now. Thank you so much for your patience 🙏🏻

@vmwclabot
Copy link

@firgavin, we have received your signed contributor license agreement. It will be reviewed by VMware shortly. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

@firgavin, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

VMware is not able to accept your contribution at this time.

@praveenrewar
Copy link
Member

Hi @firgavin I really wish I could click that merge button but it seems that the legal team won’t be able to approve your CLA based on the organisation you are working at 😞
Please feel free to reach out for more details.

@praveenrewar
Copy link
Member

Reopening

@praveenrewar praveenrewar reopened this Jan 20, 2023
@praveenrewar
Copy link
Member

Hi @firgavin! As a CNCF sandbox project, we have now moved to a vendor neutral org (carvel-dev) and are using DCO instead of VMware CLA, so would you like to rebase on develop and try to force push so that the DCO check gets triggered?

@aberres
Copy link

aberres commented Oct 11, 2024

@praveenrewar Any chance to get this feature in with @firgavin not doing the rebase?

It is not the most complex change, so are there really copyright concerns?

@100mik
Copy link
Contributor

100mik commented Oct 14, 2024

We could work towards having this change rebased and merged with a separate PR if this something you have a case for @aberres

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.

Skip checking resources when --wait=false is specified
5 participants