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

Make Deleting PVs Idempotent #1935

Merged
merged 4 commits into from
Jan 10, 2023
Merged

Make Deleting PVs Idempotent #1935

merged 4 commits into from
Jan 10, 2023

Conversation

sherifabdlnaby
Copy link
Contributor

Description

Add validation on the delete operation for PVs to make it idempotent. (extension of #1914 )

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Release Note

Release note for CHANGELOG:

Add validation on the delete operation for PVs to make it idempotent.
...

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@sherifabdlnaby sherifabdlnaby requested a review from a team as a code owner December 12, 2022 15:12
@hashicorp-cla
Copy link

hashicorp-cla commented Dec 12, 2022

CLA assistant check
All committers have signed the CLA.

@arybolovlev
Copy link
Contributor

Hi @sherifabdlnaby,

Thank you for opening this PR, however, it does not fix the issue you are referring to. This fix extends error handling returned by the method MainClientset() which seems to be the wrong place for NotFound validation.

alexsomesan
alexsomesan previously approved these changes Jan 6, 2023
Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

Thanks for spotting this. @arybolovlev is right, you are checking the error returned by the MainClientSet getter, not the actual Delete operation.

Instead, you should be doing this exact same check with the error on line 328.

@alexsomesan alexsomesan dismissed their stale review January 6, 2023 23:16

Fat-fingered the Approve button. I intended to request changes as per observations of @arybolovlev

@sherifabdlnaby
Copy link
Contributor Author

@arybolovlev Thanks for spotting my silly mistake, I moved the error check at the delete operation which should work as the PR Intended. @alexsomesan PR Is Updated & Rebased on main 👍🏻

Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

Hi @sherifabdlnaby,

All looks good, just one more query from my end.

Could you please add a changelog file so we can then auto-generate change log for the upcoming release?

It will be a simple text file in .changelog folder. Like this: .changelog/1935.txt. With the following content:

```release-note:enhancement
`resource/kubernetes_persistent_volume`: add additional validation on the delete operation to make it idempotent
```

```release-note:enhancement
`resource/kubernetes_persistent_volume_v1`: add additional validation on the delete operation to make it idempotent
```

Thank you! 👍🏻

@sherifabdlnaby sherifabdlnaby requested review from arybolovlev and removed request for alexsomesan January 10, 2023 09:35
Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

Amazing! 👍🏻 Thank you and congratulations on the first-time contribution! 🎉

Your changes will be available in the upcoming release. Please keep an eye on the release notes.

Have a great day!

@arybolovlev arybolovlev merged commit e5f0f42 into hashicorp:main Jan 10, 2023
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants