Skip to content

Introduce AtomicErr (close #175) #176

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

Merged
merged 3 commits into from
May 6, 2025
Merged

Introduce AtomicErr (close #175) #176

merged 3 commits into from
May 6, 2025

Conversation

mholt
Copy link
Contributor

@mholt mholt commented May 6, 2025

This introduces AtomicErr, and clarifies the documentation regarding atomicity. It is no longer expected/required for SetRecords, but encouraged, and AtomicErr can be returned to inform callers of the state of their zone.

@mholt mholt linked an issue May 6, 2025 that may be closed by this pull request
@worr
Copy link

worr commented May 6, 2025

Sure, this addresses my concerns. Personally, I'd drop the recommendation of synthesizing rollback, since it's error-prone especially for some providers, but I'm also not particularly bothered. Thanks a ton for addressing this, I appreciate it.

@mholt
Copy link
Contributor Author

mholt commented May 6, 2025

I will make the language a little more cautious about rollback then.

@mholt
Copy link
Contributor Author

mholt commented May 6, 2025

One last thought... should it be called AtomicError? can't decide between the "Err" pattern I've seen in Go std lib, or just spelling things out because it's not like it's a frequently used identifier.

@gucci-on-fleek gucci-on-fleek added this to the v1.1.0 milestone May 6, 2025
@gucci-on-fleek
Copy link
Collaborator

One last thought... should it be called AtomicError? can't decide between the "Err" pattern I've seen in Go std lib, or just spelling things out because it's not like it's a frequently used identifier.

I think that AtomicErr is probably better since it matches the stdlib, but AtomicError seems fine to me too.

@mholt mholt merged commit 6be5766 into master May 6, 2025
@mholt
Copy link
Contributor Author

mholt commented May 6, 2025

Sounds good, thanks both!

@mholt mholt deleted the atomic branch May 6, 2025 23:06
@gjung56
Copy link

gjung56 commented May 17, 2025

Is this pattern production ready?

I upgraded my libdns ovh package with SetRecords atomatic behavior, I used the AtomicErr type.
I used the head commit in my go.mod, until a tagged version is ready.

@gucci-on-fleek
Copy link
Collaborator

@gjung56

Is this pattern production ready?

I upgraded my libdns ovh package with SetRecords atomatic behavior, I used the AtomicErr type. I used the head commit in my go.mod, until a tagged version is ready.

Ok, I've tagged v1.1.0 just now, so you should be able to use that instead. Hopefully @mholt doesn't mind :)

@mholt
Copy link
Contributor Author

mholt commented May 17, 2025

Not at all, thank you for beating me to it!

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.

RecordSetter isn't atomic in practice
4 participants