-
Notifications
You must be signed in to change notification settings - Fork 282
Write CacheTtl to entity when add/update command is used and CacheTtl is provided #2717
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
Conversation
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
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.
Pull Request Overview
Adds tracking for whether a user explicitly provided a cache TTL option when running the update command so that this flag is correctly written back to the configuration.
- Introduces
isCacheTtlUserProvided
to record if--cache.ttl
was specified - Updates return statements to include
UserProvidedTtlOptions
based on that flag
Comments suppressed due to low confidence (2)
src/Cli/Utils.cs:846
- [nitpick] Consider renaming
isCacheTtlUserProvided
to match the property name (e.g.,userProvidedTtlOptions
) for consistency withEntityCacheOptions.UserProvidedTtlOptions
.
bool isCacheTtlUserProvided = false;
src/Cli/Utils.cs:859
- Add unit tests for scenarios when only
--cache.ttl
is provided, when only--cache.enabled
is provided, and when both are provided to verifyUserProvidedTtlOptions
is set correctly.
if (cacheTtl is not null)
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.
LGTM
…re/data-api-builder into dev/aaronburtle/CacheTtlUpdateBugFix
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
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.
From the context of this PR, this looks good to me.
However, I would wait for other reviews from the team who have better context of this change.
… is provided (#2717) ## Why make this change? Closes #2716 ## What is this change? When we create the entity cache options, we start with a new object and then later set the values for the fields. Because of this, the `bool` `UserProvidedTtlOptions` must explicitly be set when we later set these values or it will forever remain `false`. When it is false it is not written back out into the configuration file. ## How was this tested? Manually used the update command in the CLI to verify the field is written out. ## Sample Request(s) - Example REST and/or GraphQL request to demonstrate modifications - Example of CLI usage to demonstrate modifications
## Why make this change? When we create the entity cache options, we start with a new object and then later set the values for the fields. Because of this, the `bool` `UserProvidedTtlOptions` must explicitly be set when we later set these values or it will forever remain `false`. When it is false it is not written back out into the configuration file. ## What is this change? Fixes bug found in release candidate for 1.5 Cherry-picked PR: - #2717 ## How was this tested? ## Sample Request(s) Co-authored-by: aaronburtle <93220300+aaronburtle@users.noreply.github.com>
Why make this change?
Closes #2716
What is this change?
When we create the entity cache options, we start with a new object and then later set the values for the fields. Because of this, the
bool
UserProvidedTtlOptions
must explicitly be set when we later set these values or it will forever remainfalse
. When it is false it is not written back out into the configuration file.How was this tested?
Manually used the update command in the CLI to verify the field is written out.
Sample Request(s)