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

feat: add bulk method #13

Closed
wants to merge 4 commits into from
Closed

Conversation

cgroschupp
Copy link
Contributor

Because the bulk endpoint can handle create, update and delete operations, it is not necessary to have a separate method for each operation, so I implement it differently than in #9

Docs: https://desec.readthedocs.io/en/latest/dns/rrsets.html#bulk-operations

@ldez
Copy link
Member

ldez commented Nov 26, 2024

Hello,

I can see several problems with this approach:

  • the returned RRSets are not managed
  • the delete requires knowing the API (This is achieved by sending a bulk request with a RRset that has an empty records list [])

I updated PR #9, I'm still thinking about the best approach.

@ldez
Copy link
Member

ldez commented Nov 26, 2024

@cgroschupp can you check something for me?

Inside PR #9, the status code of a bulk creation is 201, I'm not sure about that, can you check it?

@cgroschupp
Copy link
Contributor Author

Only the Post request returns a status code of 201. I have changed the code.

@cgroschupp
Copy link
Contributor Author

Hello,

I can see several problems with this approach:

* the returned RRSets are not managed

* the delete requires knowing the API (This is achieved by sending a bulk request with a RRset that has an empty records list `[]`)

I updated PR #9, I'm still thinking about the best approach.

I think the first point is covered by my additional commit.
On the second point, I don't really have a good idea for changing this. One way is to add a deleted bool to the rrset and set it in bulk to [].

@ldez
Copy link
Member

ldez commented Nov 26, 2024

I merged PR #9, with some modifications.

I see you added touched field, can you create another PR dedicated to that?

@ldez
Copy link
Member

ldez commented Nov 26, 2024

I close this PR in favor of #9

@ldez ldez closed this Nov 26, 2024
@ldez ldez added the declined label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants