-
Notifications
You must be signed in to change notification settings - Fork 289
feat(rc): Add Remote Config Parameter Value Type support #591
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
3c37bde
to
896c9fa
Compare
896c9fa
to
63d2de2
Compare
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, thanks!
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.
Thanks! LGTM with a couple of nits.
assertNotEquals(parameterThree, parameterFive); | ||
assertNotEquals(parameterThree, parameterSeven); |
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.
May be also test for Equals()
with parameter value types?
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.
Line 100 has an assertEquals()
for parameter value types. Would that work?
"greeting_text", new Parameter() | ||
.setDefaultValue(ParameterValue.inAppDefault()) | ||
.setDescription("greeting text") | ||
.setConditionalValues(CONDITIONAL_VALUES) | ||
.setValueType(ParameterValueType.STRING) |
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.
Do we have coverage here for parameters without types?
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.
Good call. Updated to have a parameter without a type.
go/admin-sdk-rc-parameter-value-types
Add RC Parameter Value Type.
Update unit tests.
- Integration tests will be added following the REST API launch.- Addedrelease:stage
to trigger existing integration tests (to test backward compatibility).-Do not merge
until the BE is updated.Update integration tests.
RELEASE NOTE: Added Remote Config Parameter Value Type Support.