[FEATURE] update ACL Token Read endpoint to add expnded parameter#590
[FEATURE] update ACL Token Read endpoint to add expnded parameter#590MohamedM216 wants to merge 1 commit intoG-Research:masterfrom
Read endpoint to add expnded parameter#590Conversation
marcin-krystianc
left a comment
marcin-krystianc
left a comment
There was a problem hiding this comment.
Ww also need to implement a test for this new parameter. Ideally, in the test we call the Read operation with expended=false and expanded=true.
| Consul.Token.Read(string id, Consul.QueryOptions queryOptions, System.Threading.CancellationToken ct = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.Task<Consul.QueryResult<Consul.TokenEntry>> | ||
| Consul.Token.Read(string id, System.Threading.CancellationToken ct) -> System.Threading.Tasks.Task<Consul.QueryResult<Consul.TokenEntry>> |
There was a problem hiding this comment.
Removing an entry from that API list, means that we are breaking backwards compatibility. The user code that calls these overloads will not work anymore.
I suggest adding only one new Read overload with a new parameter and keep the other ones untouched.
There was a problem hiding this comment.
Any ideas on how to do this? If we add only one overload, how would its signature be?
- The current third overload
Read(string id, QueryOptions queryOptions, CancellationToken ct = default)is called inside other ones, if we call it again in the new overload(that hasexandedparam),expandedwill be ignored. - If we add the
expandedparam to the current third overload like thisRead(string id, QueryOptions queryOptions, bool expanded, CancellationToken ct = default), then call it instead, we will get breaking changes.
So I think we can have a scoped suppression for the warnings to mean that we're not able to change the old overload (the current third overload) as a workaround. I mean we will keep the first two overloads untouched and have pragma warning disable and pragma warning restore around the third overload. then create the new overload public async Task<QueryResult<TokenEntry>> Read(string id, QueryOptions queryOptions, bool expanded, CancellationToken ct = default) and this is the overload that will be called inside all other overloads.
Please, let me know your thoughts about this.
There was a problem hiding this comment.
Or we can change its name to ReadExpanded for example, instead of Read and get rid of all of this headache?
There was a problem hiding this comment.
I think this is the way to go:
It is not a breaking change because we provide all the necessary overloads for backwards compatibility (source and binary compatibility).
MohamedM216
force-pushed
the
update-acl-token-readendpoint
branch
3 times, most recently
from
953eb96 to
8973848
Compare
Consul.Test/TokenTest.cs
Outdated
| Assert.Equal(selfTokenEntry.Response.AccessorID, tokenEntry.Response.AccessorID); | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
If we use Skip.If in the test, then the attribute that needs to be uses is called SkippableFact
marcin-krystianc
left a comment
marcin-krystianc
left a comment
There was a problem hiding this comment.
The changes look good, but the test do not pass. Please investigate and fix.
add unit testfor expanded parameter fix tests polishing style: fix whitespaces fix test
MohamedM216
force-pushed
the
update-acl-token-readendpoint
branch
from
93ca936 to
8ec04e9
Compare
|
please see #601 |
2 participants
Description
update ACL Token
Readendpoint to addexpndedparameterRelated Issues
#531
Additional Context
Checklist
Please make sure to check the following before submitting your pull request: