Introductory PR for lots of changes#31
Open
rrichardson wants to merge 14 commits into
Open
Conversation
Contributor
|
Hi @rrichardson We would love to integrate your changes but the scope of them is quite large and a bit risky to pull off all in one go (since we use this library internally). If you could sign the CLA, I would be happy to try to pull these changes in piecemeal. |
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
I have read the CLA Document and I hereby sign the CLA |
|
I also linked my email so that should be fixed as well |
Author
|
I have read the CLA Document and I hereby sign the CLA |
…ests to make them idempotent
…alog/service nodes list as the default, also adding get single NodeFull by service and name
Make the Service port field optional
|
What's the status of this? If it was integrated bit-by-bit, shouldn't the PR be closed? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem are we solving?
I have been building some systems against this library for the last couple of days and made a bunch of changes to my fork. These changes include additional functions, but also updates the rs-consul code to make it more "idiomatic Rust".
A couple of these changes do break backwards compatibility a bit, but not a lot. This would probably warrant a new minor version.
This is a lot of changes, so it might need to be broken up into a few separate PRs, which I'm willing to do. I'd just like to put this out there to get some feedback first.
Here is an incomplete list of changes:
Introduced a
Base64VectypeThis is mostly used under the hood to simplify sending
Vec<u8>to and from Consul. It transparently base64 encodes and decodes with a custom Serializer and Deserializer.ReadKeyRequestis generic over T (see below) but defaults to Base64Vec.Changed all KV related operations to be generic over type T
Previously one had to write
Vec<u8>and read aString, which I found to be a little awkward.In all cases, T must implement
SerializeorDeserialize+Debug+Defaultand in some cases (get_lock) it requresCloneas well.If all you're doing is storing and retrieving
Stringthis interface should be a bit simpler, because you don't have to convert to and from VecMethods updated to be Generic over T:
Structs updated to be generic over T:
Added Transaction operations
You can now send batches of operations to be executed with (some?) isolation using Consuls txn API. (see https://developer.hashicorp.com/consul/api-docs/txn)
This includes new a new method:
create_or_update_allwhich takes a Vec ofTransactionOp. Note that I didn't add support for all request types into TransactionOp, only KV, which is all that I needed.Note that TransactionOp is not generic over T, because A
Vec<TransactionOp<T>>would allow only 1 type of value to be written. So it takes a Base64Vec as a value instead.Tests are idempotent, for the most part
The tests assumed that the database was pristine, so some tests would fail on multiple runs. This is no longer the case. They now clean up before their operations.
Exposed
create_sessionand made aget_lock_innerFor those that don't want to use a LockGuard with
dropsemantics, because they have their own system for managing locks, I've exposed the inner workings ofget_lockMade everything more self-consistent and idiomatic Rust
*Payloadnow they're all called*RequestStrings while others took&str- Now they all take &strallowattr. These have been changed to snake_case, and useserde_rename = "PascalCase"instead, like the other structs.Checks
Please check these off before promoting the pull request to non-draft status.