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 ability to delete multi-value records in Route 53 #988

Closed
wants to merge 4 commits into from

Conversation

vaygr
Copy link
Contributor

@vaygr vaygr commented Feb 15, 2017

Ability to delete multi-value records in Route 53

Description

Currently any .delete() operation on multi-value records results in RecordDoesNotExistError. This PR handles record deletion correctly regardless of its type.

Status

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

@tonybaloney
Copy link
Contributor

please add a supporting test to validate this feature.

thanks for your contribution

@pquentin
Copy link
Contributor

@vaygr Any help needed to write the test?

@vaygr
Copy link
Contributor Author

vaygr commented Jan 29, 2018

Ah, no, I think I'm good, it just seemed to drop off my list. I'll update the PR shortly.

@pquentin
Copy link
Contributor

@vaygr ping!

@vaygr
Copy link
Contributor Author

vaygr commented Apr 7, 2018

@pquentin sorry, it slipped again.

I added the test, but while doing this I discovered that current one (test_delete_record()) doesn't cover the corresponding operation fully: it doesn't catch the error for multi-value record deletion and I couldn't find a way to check whether the record was actually deleted or not afterwards.

Suggestions?

@pquentin
Copy link
Contributor

What error do you want to handle? You can control the response you get in tests for the same API call by setting Route53MockHttp.type: test_delete_record_does_not_exist is a test that does this.

Please tell me if that helps or not.

@stale
Copy link

stale bot commented Jul 16, 2019

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue.

@stale stale bot added the stale label Jul 16, 2019
@stale stale bot removed the stale label Jul 18, 2019
@Kami
Copy link
Member

Kami commented Jul 18, 2019

First thank your for your contribution.

This PR is very old and out of date now so I will go ahead and close it.

If it's still relevant, please sync your changes with the latest master / trunk and open a new PR. Thanks!

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

Successfully merging this pull request may close these issues.

4 participants