fix(types)!: improve types in index.ts by AVaksman · Pull Request #720 · googleapis/nodejs-bigtable

AVaksman · 2020-04-23T21:25:42Z

BREAKING CHANGE: cluster.setMetadata(): only node count is updatable on an existing cluster; getInstancesCallback/Response: dropped nextQuery property as it is deprecated for this method, exposed failedLocations property; instance.createCluster(): removed unsupported params serveNodes and defaultStorageType

Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Improve type annotations, some clean up and minor refactoring.

codecov · 2020-04-23T21:31:02Z

Codecov Report

Merging #720 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #720      +/-   ##
==========================================
+ Coverage   99.23%   99.28%   +0.04%     
==========================================
  Files          15       15              
  Lines       15735    15724      -11     
  Branches      953      952       -1     
==========================================
- Hits        15615    15611       -4     
+ Misses        117      110       -7     
  Partials        3        3

Impacted Files	Coverage Δ
src/cluster.ts	`100.00% <100.00%> (+0.65%)`	⬆️
src/index.ts	`100.00% <100.00%> (+0.17%)`	⬆️
src/instance.ts	`100.00% <100.00%> (+0.17%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21f8fef...db97e9f. Read the comment docs.

AVaksman · 2020-04-23T21:38:16Z

src/cluster.ts

  apiResponse?: google.bigtable.admin.v2.IListClustersResponse
 ) => void;
+export interface SetClusterMetadataOptions {
+  nodes: number;


Only nodes are updatable in cluster after it's creation

AVaksman · 2020-04-23T21:39:44Z

src/cluster.ts

-    if (metadata.location) {
-      reqOpts.location = Cluster.getLocation_(
-        this.bigtable.projectId,
-        metadata.location
-      );
-    }
-
-    if (metadata.nodes) {
-      reqOpts.serveNodes = metadata.nodes;
-    }
-
-    if (metadata.storage) {
-      reqOpts.defaultStorageType = Cluster.getStorageType_(metadata.storage);
-    }
-


storage and location can not be updated on an existing cluster

I haven't tested this or anything, but the docs say that the UpdateCluster rpc accepts a full "Cluster" object: https://cloud.google.com/bigtable/docs/reference/admin/rpc/google.bigtable.admin.v2#google.bigtable.admin.v2.BigtableInstanceAdmin.UpdateCluster-table

Will it return an error if certain fields are attempted to be updated?

It won't return an error,
This change is done to eliminate a possibility of a confusion from the user side if attempted to change a filed that is not supported.

I see, thanks. I'm not sure we should use the whitelist approach of only allowing "serveNodes", when the API could evolve someday to accept something else, and we'd be blocking it. Does the upstream team know their API docs state multiple unsupported properties can be sent during the UpdateCluster call, and maybe they would want to make it return an error from their end, or some other solution?

We could also throw from our library if we receive unsupported input, but in general, we're best to simply match what the official docs state is supported. As soon as we go off that path, things get shaky.

Does the upstream team know their API docs state multiple unsupported properties can be sent during the UpdateCluster call

While UpdateCluster rpc does accept a full "Cluster" object, it seems that upstream API docs do specify that among Cluster properties

name is an OutputOnly field

state is an OutputOnly field as well

location is a CreateOnly field

default_storage_type is a CreateOnly field as well

leaving only serveNodes field that can be updated

when the API could evolve someday to accept something else, and we'd be blocking it

If/when upstream API evolves to accept something else, that logic could be added as a new feature PR?

WDYT?

If/when upstream API evolves to accept something else, that logic could be added as a new feature PR?

There's usually a big lag before we notice.

If the upstream API silently drops properties, I would think our best bet is to do the same. In terms of code, let's pass them all through. In terms of types, maybe that's where you could just list "serveNodes"? At least then, if the user knows better and is trying to set a property we haven't added to the types yet, they could have their TypeScript compiler ignore that (I think?).

Added the logic back, left the types with nodes only.

@stephenplusplus WDYT?

AVaksman · 2020-04-23T21:45:43Z

src/index.ts

-    nextQuery?: {} | null,
-    response?: google.bigtable.admin.v2.IListInstancesResponse | null
+    result?: Instance[],
+    failedLocations?: string[],


BREAKING CHANGE
exposing failedLocations in the callback/response

Let's make sure we add docs for this new response argument.

Also, an example in the code block showing "if (failedLocations.length > 0) { // These locations contain instances which could not be retrieved. }"

AVaksman · 2020-04-23T21:49:34Z

src/instance.ts

-export interface ClusterInfo {
-  id?: string;
-  location?: string;
-  serveNodes?: number;
-  nodes?: number;
-  storage?: string;
-  defaultStorageType?: number;


BREAKING CHANGE
Removing serveNodes and defaultStorageType as these are expected by GAPIC client, user facing parameters are nodes and storage respectively.

bcoe

I don't think our parser is smart enough for BREAKING CHANGES, to pull in the bullets. So I'd just do:

BREAKING CHANGE: cluster.setMetadata(): only node count is updatable on an existing cluster; getInstancesCallback/Response: dropped nextQuery property as it is deprecated for this method; etc

JustinBeckwith · 2020-05-27T22:52:56Z

@AVaksman we prolly want to land this before shipping v3, right? Can I trouble you to run through the open PRs and try to get these ready to go?

AVaksman · 2020-06-02T18:39:17Z

BREAKING CHANGE:

cluster.setMetadata()
- only node count is updatable on an existing cluster
- removed location and storage from the params
getInstancesCallback/Response
- exposed failedLocations property that is also returned along with array of Instnaces
- dropped nextQuery property as it is deprecated for this method
createInstance(id, options)
- made options into a required param as it is not possible to create an instance without providing at least one cluster (part of options param)
createCluster
- removed duplicated, unsupported (by implementation), undocumented params serveNodes and defaultStorageType in favor of nodes and location respectfully

stephenplusplus · 2020-06-05T16:02:22Z

src/cluster.ts


-    if (metadata.location) {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    if ((metadata as any).location) {


If the API actually drops these parameters, I think we can skip all of the logic where we worry about formatting them.

stephenplusplus · 2020-06-05T16:02:44Z

src/cluster.ts


    const reqOpts: ICluster = {
      name: this.name,
+      serveNodes: metadata.nodes,


This is where I think we should pass all user-provided input straight through, as opposed to the whitelist approach where we only recognize certain parameters.

(mapping metadata.nodes to serveNodes is still good)

If applicable, user will need to pass
the location as a full path in the form of
projects/<project>/locations/<zone_id>

and a defaultStorageType (not storage) param with a value in upper case characters.

Done

test/cluster.ts

src/index.ts

AVaksman · 2020-06-11T21:00:59Z

@stephenplusplus before merging, does PR description look OK?

BREAKING CHANGE: cluster.setMetadata(): only node count is updatable on an existing cluster; getInstancesCallback/Response: dropped nextQuery property as it is deprecated for this method, exposed failedLocations property; instance.createCluster(): removed unsupported params serveNodes and defaultStorageType

stephenplusplus · 2020-06-12T14:39:31Z

It looks good to me!

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 23, 2020

AVaksman commented Apr 23, 2020

View reviewed changes

AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 24, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 24, 2020

AVaksman added 3 commits May 15, 2020 15:26

fix(types): getInstances

0a5a972

fix(types): createInstance

3bb5f9a

fix: coverage

2cf8704

AVaksman force-pushed the update_types branch from 625d609 to 2cf8704 Compare May 15, 2020 19:26

fix: patch types

f0b7938

AVaksman mentioned this pull request May 18, 2020

fix(types)!: improve types for instance.ts #731

Closed

3 tasks

bcoe approved these changes May 22, 2020

View reviewed changes

Merge branch 'master' into update_types

cd429f6

AVaksman and others added 2 commits June 3, 2020 23:29

Merge branch 'master' into update_types

5f3399e

feat: pass through location and storage

bd3c77d

stephenplusplus reviewed Jun 5, 2020

View reviewed changes

AVaksman added 2 commits June 8, 2020 13:55

chode: update docs

66c480a

fix: pass all user-provided input in cluster.setMetadata()

859000b

stephenplusplus suggested changes Jun 9, 2020

View reviewed changes

test/cluster.ts Outdated Show resolved Hide resolved

src/index.ts Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

AVaksman added 3 commits June 9, 2020 11:31

chore: fix typo

79276e8

chore: fix docs

414afa2

feat: throw if at least one cluster configuration was not provided

75eb91a

stephenplusplus reviewed Jun 9, 2020

View reviewed changes

src/index.ts Show resolved Hide resolved

feat: throw if configuration object was not provided

ec30c7f

stephenplusplus approved these changes Jun 9, 2020

View reviewed changes

AVaksman added 2 commits June 10, 2020 11:12

Merge branch 'master' into update_types

4499de2

Merge branch 'master' into update_types

5b39add

Merge branch 'master' into update_types

db97e9f

AVaksman merged commit 508d1f9 into googleapis:master Jun 15, 2020

Conversation

AVaksman commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

JustinBeckwith commented May 27, 2020

Uh oh!

AVaksman commented Jun 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AVaksman Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AVaksman commented Jun 11, 2020

Uh oh!

stephenplusplus commented Jun 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AVaksman commented Apr 23, 2020 •

edited

Loading

codecov bot commented Apr 23, 2020 •

edited

Loading

AVaksman Jun 8, 2020 •

edited

Loading