Add Remote Config Rollback operation #885

lahirumaramba · 2020-05-11T19:02:46Z

Add rollback operation to the RC API
Add unit tests

Note: The API proposal is still under review. Getting a head start here.

hiranya911

Looks great! Just a couple of nits.

hiranya911 · 2020-05-11T20:07:13Z

src/index.d.ts

@@ -1025,6 +1025,18 @@ declare namespace admin.remoteConfig {
     */
    publishTemplate(template: RemoteConfigTemplate, options?: { force: boolean }): Promise<RemoteConfigTemplate>;

+    /**
+    * Rollbacks a project's published Remote Config template to the one specified by the provided


"... to the specified version."

hiranya911 · 2020-05-11T20:08:19Z

src/remote-config/remote-config-api-client.ts

+        return this.httpClient.send(request);
+      })
+      .then((resp) => {
+        if (!validator.isNonEmptyString(resp.headers['etag'])) {


I'd assume this logic is used by at least one other method (getTemplate). Extract into a helper method?

Good catch! Moved to a helper method. Thanks!

lahirumaramba · 2020-05-11T22:10:33Z

Hi @egilmorez ! Please review the docstrings in index.d.ts. Thank you!

egilmorez

Some things to look at Lahiru, thanks!

egilmorez · 2020-05-11T22:14:51Z

src/index.d.ts

@@ -1025,6 +1025,18 @@ declare namespace admin.remoteConfig {
     */
    publishTemplate(template: RemoteConfigTemplate, options?: { force: boolean }): Promise<RemoteConfigTemplate>;

+    /**
+    * Rollbacks a project's published Remote Config template to the specified version.


Despite the noun "rollback" we still would way "Rolls back a project's . . . "

egilmorez · 2020-05-11T22:18:03Z

src/index.d.ts

+    /**
+    * Rollbacks a project's published Remote Config template to the specified version.
+    * A rollback is equivalent to getting a previously published Remote Config
+    * template, and re-publishing it using a force update.


Suggest omitting the comma here.

egilmorez · 2020-05-11T22:18:34Z

src/index.d.ts

+    * template, and re-publishing it using a force update.
+    * 
+    * @param versionNumber The version number of the Remote Config template to rollback to.
+    *    The specified version number must be less than the current version number, and not have


Suggest "lower than"

lahirumaramba · 2020-05-11T22:30:44Z

Thanks @egilmorez ! Updated.

hiranya911

One tiny change related to the shared util.

hiranya911 · 2020-05-11T22:35:53Z

src/remote-config/remote-config-api-client.ts

-            'ETag header is not present in the server response.');
-        }
+        this.validateResponseEtag(resp.headers['etag']);
+        return {


This can also go into a util right? Something like:

return this.createRemoteConfigTemplate(resp);

The util can do the validation and then extract the right properties from the response.

Great point! Moved the creation to a new util. Thanks!

hiranya911

LGTM with a couple of nits to think about.

hiranya911 · 2020-05-13T18:12:43Z

src/remote-config/remote-config-api-client.ts

+      conditions: resp.data.conditions,
+      parameters: resp.data.parameters,
+      parameterGroups: resp.data.parameterGroups,
+      etag: etag,


Nit: You can just do etag, here.

hiranya911 · 2020-05-13T18:21:55Z

src/remote-config/remote-config-api-client.ts

@@ -140,16 +134,11 @@ export class RemoteConfigApiClient {
    this.validateRemoteConfigTemplate(template);
    return this.sendPutRequest(template, template.etag, true)
      .then((resp) => {
+        // validating a template returns an etag with the suffix -0 means that your update 


Do we ever check for this -0 suffix?

We don't necessarily check for -0 suffix here. If the response is not an error (200 OK) and contains a valid etag, we can safely assume that the template was successfully validated. All other cases should return an error code. I will add an additional check for the -0 suffix in a separate PR for good measure :)

hiranya911 · 2020-05-13T18:23:13Z

src/remote-config/remote-config-api-client.ts

+   */
+  private toRemoteConfigTemplate(resp: HttpResponse, customEtag?: string): RemoteConfigTemplate {
+    const etag = (typeof customEtag == 'undefined') ? resp.headers['etag'] : customEtag;
+    this.validateResponseEtag(etag);


Nit: This should be called something like validateEtag() now.

* Get remote config template by version number (#874) * Get remote config template by version number * Refactor unit tests for Remote Config (#884) * Refactor unit tests * Add Remote Config Rollback operation (#885) * 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 (#896) * 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 a separate function to get a Template with version (#902) * Add version meta data to RC templates (#906) * Add version meta data to RC templates * PR fixes * Use assertion to unwrap template.version * Update Remote Config Docstrings in index.d.ts

lahirumaramba requested a review from hiranya911 May 11, 2020 19:02

lahirumaramba assigned hiranya911 May 11, 2020

Add Remote Config Rollback operation

1a43947

lahirumaramba force-pushed the lm-rc-v2-rollback branch from 2234b10 to 1a43947 Compare May 11, 2020 19:03

hiranya911 suggested changes May 11, 2020

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 May 11, 2020

Update docs and move etag validation to a helper function

9c41146

lahirumaramba assigned hiranya911 and unassigned lahirumaramba May 11, 2020

lahirumaramba requested a review from egilmorez May 11, 2020 22:10

lahirumaramba assigned egilmorez May 11, 2020

egilmorez reviewed May 11, 2020

View reviewed changes

Update docs

e8e180e

hiranya911 suggested changes May 11, 2020

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 May 11, 2020

Introduce a util to create a template from API response

cfa61a5

lahirumaramba assigned hiranya911 and unassigned egilmorez and lahirumaramba May 12, 2020

hiranya911 approved these changes May 13, 2020

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 May 13, 2020

PR fixes

915a4ef

lahirumaramba assigned lahirumaramba and unassigned lahirumaramba May 15, 2020

lahirumaramba merged commit 5762470 into remote-config-vc May 15, 2020

lahirumaramba deleted the lm-rc-v2-rollback branch May 15, 2020 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Remote Config Rollback operation #885

Add Remote Config Rollback operation #885

lahirumaramba commented May 11, 2020 •

edited

hiranya911 left a comment

hiranya911 May 11, 2020

hiranya911 May 11, 2020

lahirumaramba May 11, 2020

lahirumaramba commented May 11, 2020

egilmorez left a comment

egilmorez May 11, 2020

egilmorez May 11, 2020

egilmorez May 11, 2020

lahirumaramba commented May 11, 2020

hiranya911 left a comment

hiranya911 May 11, 2020

lahirumaramba May 12, 2020

hiranya911 left a comment

hiranya911 May 13, 2020

lahirumaramba May 15, 2020

hiranya911 May 13, 2020

lahirumaramba May 15, 2020

hiranya911 May 13, 2020

Add Remote Config Rollback operation #885

Add Remote Config Rollback operation #885

Conversation

lahirumaramba commented May 11, 2020 • edited

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahirumaramba commented May 11, 2020

egilmorez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahirumaramba commented May 11, 2020

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahirumaramba commented May 11, 2020 •

edited