-
Notifications
You must be signed in to change notification settings - Fork 409
Add support for Remote Config parameter groups #840
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
lahirumaramba
commented
Apr 6, 2020
- Add support for RC Parameter Groups
- Add unit tests
- Add integration tests
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.
Loos good. Just one question, and make sure we have test coverage for the case where the groups are either empty or undefined on the response.
@hiranya911 Thanks! Added more unit tests to cover cases where |
@egilmorez please review the docs for Remote Config Parameter Groups. Thank you!! |
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
}); | ||
}); | ||
|
||
it('should resolve with conditions:[] when no conditions present in the response', () => { |
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.
There's a lot of redundancy in these tests. As a future improvement, think about ways to clean it up a bit.
src/index.d.ts
Outdated
|
||
/** | ||
* Map of parameter group names to their parameter group objects. | ||
* A group's name is mutable but must be unique among groups in the config. |
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.
Not sure, but where we say "the config" here, do we mean the Remote Config template? Or maybe the RemoteConfig
object, as you mention it in line 891?
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 catch. It should be Remote Config template
. Updated in both places. 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.
LG! Just one question, thanks.