-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[App Config] updateSyncToken method #14507
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small changes.
{ syncTokens } as InternalAppConfigurationClientOptions | ||
); | ||
assert.equal( | ||
client["_syncTokens"]["_currentSyncTokens"].size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You passed in syncTokens
here so you don't need to delve into the object private fields to get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client["_syncTokens"] -> syncTokens
// user treats token as an opaque value (note - this _does_ mutate the appconfigclient) | ||
// however, this mutations happens regardless since the system can and does return | ||
// sync token headers during normal operation. | ||
client.updateSyncToken(eventGridEvent.syncToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check - there's nothing to test here. I'd remove this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
…to harshan/app-config/sync-token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Hello @HarshaNalluru! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
### TODOs - [x] Add the new `updateSyncToken` method to `AppConfigurationClient` (Picked up pieces from @richardpark-msft's work here Azure#14342.) - [x] Add a unit test - [x] ~~Hoping to add a live test by leveraging the eventgrid SDK~~ - [x] Changelog - [x] Hold off merging - wait for 1.1.1 to be released - [x] What's the new version number for preview? - `1.2.0-beta.1` fine? Reference: PR from .NET Azure/azure-sdk-for-net#18487
TODOs
updateSyncToken
method toAppConfigurationClient
(Picked up pieces from @richardpark-msft's work here [App Config] FeatureFlag and SecretReference support #14342.)
Hoping to add a live test by leveraging the eventgrid SDK1.2.0-beta.1
fine?Reference: PR from .NET Azure/azure-sdk-for-net#18487