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 kv txn get-or-empty operation. #14474

Merged
merged 1 commit into from
Sep 6, 2022
Merged

Add kv txn get-or-empty operation. #14474

merged 1 commit into from
Sep 6, 2022

Conversation

hashi-derek
Copy link
Member

@hashi-derek hashi-derek commented Sep 2, 2022

Description

Adds a get-or-empty operation to the kv txn api. There is currently no operation for fetching the value of a key that may not yet exist. The current get command will cause the entire transaction to roll-back if the key doesn't exist. The new op will return a nil value for a key if-not-found.

https://www.consul.io/api-docs/txn#kv-operations

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@github-actions github-actions bot added theme/api Relating to the HTTP API interface type/docs Documentation needs to be created/updated/clarified labels Sep 2, 2022
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Amazing job adding this small enhancement @hashi-derek 👏

Overall this is exactly what I imagined too when thinking about this problem.

I have a couple of minor nits inline which I don't think block merging this.

My biggest bit of feedback would be on the name chosen. I think get-if-exists is slightly more clear/descriptive of the behaviour vs. get-not-exists. I see how there is a consistency argument with check-not-exists but I think that makes more sense in context "Check that key X does not exist" is naturally expressed by check-not-exists whereas "Get the value of key X if it exists or empty otherwise" requires a double-negative to map to get-not-exists and maps more naturally to get-if-exists in my mind anyway 😄.

If you disagree though it'd not a mountain I feel the need to die on! Should be a quick enough update if you do!

agent/consul/state/txn.go Show resolved Hide resolved
agent/consul/state/txn_test.go Show resolved Hide resolved
website/content/api-docs/txn.mdx Outdated Show resolved Hide resolved
@kisunji
Copy link
Contributor

kisunji commented Sep 6, 2022

Amazing job adding this small enhancement @hashi-derek 👏

Overall this is exactly what I imagined too when thinking about this problem.

I have a couple of minor nits inline which I don't think block merging this.

My biggest bit of feedback would be on the name chosen. I think get-if-exists is slightly more clear/descriptive of the behaviour vs. get-not-exists. I see how there is a consistency argument with check-not-exists but I think that makes more sense in context "Check that key X does not exist" is naturally expressed by check-not-exists whereas "Get the value of key X if it exists or empty otherwise" requires a double-negative to map to get-not-exists and maps more naturally to get-if-exists in my mind anyway 😄.

If you disagree though it'd not a mountain I feel the need to die on! Should be a quick enough update if you do!

How about get-or-nil or get-or-empty? Semantically, get-if-exists isn't descriptive of the "if not exists" case (it could very well describe get)

@hashi-derek
Copy link
Member Author

@kisunji Yeah, I wasn't a fan of any of the names I had come up with. get-or-empty seems like a reasonable choice. Since the action will always return a result, it's probably more clear than get-if-exists as well.

Copy link
Contributor

@kisunji kisunji left a comment

Choose a reason for hiding this comment

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

👍

@hashi-derek hashi-derek merged commit 02ae66b into main Sep 6, 2022
@hashi-derek hashi-derek deleted the derekm/add-kv-get branch September 6, 2022 15:29
@hashi-derek hashi-derek changed the title Add kv txn get-not-exists operation. Add kv txn get-or-empty operation. Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/api Relating to the HTTP API interface type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants