-
Notifications
You must be signed in to change notification settings - Fork 391
feat(rc): Add Remote Config Version Management API #920
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
* Get remote config template by version number
* Refactor unit tests
* Add Remote Config Rollback operation * Update docs and move etag validation to a helper function * Update docs * Introduce a util to create a template from API response * PR fixes
* Remote Config: Add list versions operation * Add version Impl and other PR fixes * PR fixes * Imrpoved unit tests and some code clean up * Fix code formatting
* Add version meta data to RC templates * PR fixes * Use assertion to unwrap template.version
Integration tests will be added once we merge #914 |
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.
Some things for you to look at, thanks Lahiru!
src/index.d.ts
Outdated
@@ -878,6 +878,11 @@ declare namespace admin.remoteConfig { | |||
* ETag of the current Remote Config template (readonly). | |||
*/ | |||
readonly etag: string; | |||
|
|||
/** | |||
* Version information of the current Remote Config template. |
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.
Does this return just a number?
I lean toward "information for" unless it's just a "version number of"
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! It is not just a number. Changed to information for
.
src/index.d.ts
Outdated
* Interface representing a Remote Config template version. | ||
* Output only, except for the version description. Contains metadata about a particular | ||
* version of the Remote Config template. All fields are set at the time the specified Remote | ||
* Config template was published. A version's description field may be specified in |
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.
Suggest "is published"
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.
Updated!
src/index.d.ts
Outdated
|
||
/** | ||
* The timestamp of when this version of the Remote Config template was written to the | ||
* Remote Config server. |
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.
Suggest "backend" instead of server, unless it's inaccurate. We use backend a lot in the guide docs.
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. backend
makes more sense here.
src/index.d.ts
Outdated
updateTime?: string; | ||
|
||
/** | ||
* The source of origin of the template update action. |
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.
Suggest just "origin"
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.
Updated
*/ | ||
description?: 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.
Suggest a bit of a rewrite:
"The version number of
the Remote Config template that has become the current version due to a rollback. Only present if this version is the result of a rollback."
Is this accurate? :)
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.
This is accurate! Thanks. Updated.
src/index.d.ts
Outdated
pageToken?: string; | ||
|
||
/** | ||
* Specify the newest version number to include in the results. |
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.
Suggest "Specifies" here and below.
src/index.d.ts
Outdated
* A rollback is equivalent to getting a previously published Remote Config | ||
* template and re-publishing it using a force update. | ||
* | ||
* @param versionNumber The version number of the Remote Config template to rollback to. |
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.
Suggest "roll back to"
src/index.d.ts
Outdated
* | ||
* @param versionNumber The version number of the Remote Config template to rollback to. | ||
* The specified version number must be lower than the current version number, and not have | ||
* been deleted due to staleness. |
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.
What's the limit on this? 300 versions/90 days?
Might be good to mention here, not sure. It's right below as well :)
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.
Added 300 versions/90 days text here as well
src/index.d.ts
Outdated
/** | ||
* Gets a list of Remote Config template versions that have been published, sorted in reverse | ||
* chronological order. Only the last 300 versions are stored. | ||
* All versions that correspond to non-active Remote Config templates (i.e., all except the |
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.
Suggest "that is, all except . . ."
src/index.d.ts
Outdated
* template that is being fetched by clients) are also deleted if they are older than 90 days. | ||
* | ||
* @param options Optional {@link admin.remoteConfig.ListVersionsOptions `ListVersionsOptions`} | ||
* object for getting a list of tempalte versions. |
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.
Typo.
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! Thanks!
- Update Remote Config Docstrings in index.d.ts
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 Lahiru! All good from a docs perspective.
RELEASE NOTES: Added version management support for
admin.remoteConfig()
API. This API now supportslistVersions()
,getTemplateAtVersion()
, androllback()
operations to help developers programmatically manage their Remote Config templates.