Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(types)!: improve types in index.ts #720

Merged
merged 16 commits into from
Jun 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 25 additions & 31 deletions src/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,21 @@ export type GetClustersCallback = (
clusters?: Cluster[],
apiResponse?: google.bigtable.admin.v2.IListClustersResponse
) => void;
export interface SetClusterMetadataOptions {
nodes: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nodes are updatable in cluster after it's creation

}
export type SetClusterMetadataCallback = GenericOperationCallback<
Operation | null | undefined
>;

export interface CreateClusterOptions {
gaxOptions?: CallOptions;
location?: string;
export interface BasicClusterConfig {
location: string;
nodes: number;
storage?: string;
}

export interface CreateClusterOptions extends BasicClusterConfig {
gaxOptions?: CallOptions;
}
export type GetClusterMetadataCallback = (
err: ServiceError | null,
metadata?: ICluster | null,
Expand Down Expand Up @@ -367,18 +372,15 @@ Please use the format 'my-cluster' or '${instance.name}/clusters/my-cluster'.`);
}

setMetadata(
metadata: CreateClusterOptions
metadata: SetClusterMetadataOptions,
gaxOptions?: CallOptions
): Promise<SetClusterMetadataResponse>;
setMetadata(
metadata: CreateClusterOptions,
gaxOptions: CallOptions
): Promise<SetClusterMetadataResponse>;
setMetadata(
metadata: CreateClusterOptions,
metadata: SetClusterMetadataOptions,
callback: SetClusterMetadataCallback
): void;
setMetadata(
metadata: CreateClusterOptions,
metadata: SetClusterMetadataOptions,
gaxOptions: CallOptions,
callback: SetClusterMetadataCallback
): void;
Expand All @@ -399,35 +401,27 @@ Please use the format 'my-cluster' or '${instance.name}/clusters/my-cluster'.`);
* region_tag:bigtable_cluster_set_meta
*/
setMetadata(
metadata: CreateClusterOptions,
metadata: SetClusterMetadataOptions,
gaxOptionsOrCallback?: CallOptions | SetClusterMetadataCallback,
cb?: SetClusterMetadataCallback
): void | Promise<SetClusterMetadataResponse> {
const callback =
typeof gaxOptionsOrCallback === 'function' ? gaxOptionsOrCallback : cb!;
const gaxOptions =
typeof gaxOptionsOrCallback === 'object' && gaxOptionsOrCallback
typeof gaxOptionsOrCallback === 'object'
? gaxOptionsOrCallback
: ({} as CallOptions);

const reqOpts: ICluster = {
name: this.name,
};

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);
}
const reqOpts: ICluster = Object.assign(
{},
{
name: this.name,
serveNodes: metadata.nodes,
},
metadata
);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
delete (reqOpts as any).nodes;

this.bigtable.request<Operation>(
{
Expand Down
64 changes: 32 additions & 32 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import {
InstanceOptions,
CreateInstanceCallback,
CreateInstanceResponse,
ClusterInfo,
IInstance,
} from './instance';
import {shouldRetryRequest} from './decorateStatus';
Expand All @@ -49,14 +48,14 @@ const {grpc} = new gax.GrpcClient();
export interface GetInstancesCallback {
(
err: ServiceError | null,
result?: Instance[] | null,
nextQuery?: {} | null,
response?: google.bigtable.admin.v2.IListInstancesResponse | null
result?: Instance[],
failedLocations?: string[],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BREAKING CHANGE
exposing failedLocations in the callback/response

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

response?: google.bigtable.admin.v2.IListInstancesResponse
): void;
}
export type GetInstancesResponse = [
Instance[],
{} | null,
string[],
google.bigtable.admin.v2.IListInstancesResponse
];

Expand Down Expand Up @@ -462,14 +461,13 @@ export class Bigtable {

createInstance(
id: string,
options?: InstanceOptions
options: InstanceOptions
): Promise<CreateInstanceResponse>;
createInstance(
id: string,
options: InstanceOptions,
callback: CreateInstanceCallback
): void;
createInstance(id: string, callback: CreateInstanceCallback): void;
/**
* Create a Cloud Bigtable instance.
*
Expand All @@ -479,7 +477,7 @@ export class Bigtable {
* @param {object} options Instance creation options.
* @param {object[]} options.clusters The clusters to be created within the
* instance.
* @param {string} options.displayName The descriptive name for this instance
* @param {string} [options.displayName] The descriptive name for this instance
* as it appears in UIs.
* @param {Object.<string, string>} [options.labels] Labels are a flexible and
* lightweight mechanism for organizing cloud resources into groups that
Expand Down Expand Up @@ -546,14 +544,19 @@ export class Bigtable {
*/
createInstance(
id: string,
optionsOrCallback?: InstanceOptions | CreateInstanceCallback,
cb?: CreateInstanceCallback
options: InstanceOptions,
callback?: CreateInstanceCallback
): void | Promise<CreateInstanceResponse> {
const options =
typeof optionsOrCallback === 'object' ? optionsOrCallback : {};
const callback =
typeof optionsOrCallback === 'function' ? optionsOrCallback : cb;

if (typeof options !== 'object') {
throw new Error(
'A configuration object is required to create an instance.'
);
}
if (!options.clusters) {
throw new Error(
'At least one cluster configuration object is required to create an instance.'
);
}
const reqOpts = {
parent: this.projectName,
instanceId: id,
Expand All @@ -567,7 +570,7 @@ export class Bigtable {
reqOpts.instance!.type = Instance.getTypeType_(options.type);
}

reqOpts.clusters = arrify(options.clusters!).reduce((clusters, cluster) => {
reqOpts.clusters = arrify(options.clusters).reduce((clusters, cluster) => {
if (!cluster.id) {
throw new Error(
'A cluster was provided without an `id` property defined.'
Expand All @@ -581,7 +584,7 @@ export class Bigtable {
};

return clusters;
}, {} as {[index: string]: ClusterInfo});
}, {} as {[index: string]: google.bigtable.admin.v2.ICluster});

this.request(
{
Expand All @@ -606,12 +609,14 @@ export class Bigtable {
/**
* @typedef {array} GetInstancesResponse
* @property {Instance[]} 0 Array of {@link Instance} instances.
* @property {object} 1 The full API response.
* @property {string[]} 1 locations from which Instance information could not be retrieved
* @property {object} 2 The full API response.
*/
/**
* @callback GetInstancesCallback
* @param {?Error} err Request error, if any.
* @param {Instance[]} instances Array of {@link Instance} instances.
* @param {string[]} locations from which Instance information could not be retrieved
* @param {object} apiResponse The full API response.
*/
/**
Expand All @@ -629,26 +634,21 @@ export class Bigtable {
* bigtable.getInstances(function(err, instances) {
* if (!err) {
* // `instances` is an array of Instance objects.
* if (failedLocations.length > 0) {
* // These locations contain instances which could not be retrieved.
* }
* }
* });
*
* @example <caption>To control how many API requests are made and page
* through the results manually, set `autoPaginate` to `false`.</caption>
* function callback(err, instances, nextQuery, apiResponse) {
* if (nextQuery) {
* // More results exist.
* bigtable.getInstances(nextQuery, callback);
* }
* }
*
* bigtable.getInstances({
* autoPaginate: false
* }, callback);
*
* @example <caption>If the callback is omitted, we'll return a Promise.
* </caption>
* bigtable.getInstances().then(function(data) {
* const instances = data[0];
*
* if (data[1]) {
* // These locations contain instances which could not be retrieved.
* const failedLocations = data[1];
* }
* });
*/
getInstances(
Expand Down Expand Up @@ -683,7 +683,7 @@ export class Bigtable {
instance.metadata = instanceData;
return instance;
});
callback!(null, instances, resp);
callback!(null, instances, resp.failedLocations, resp);
}
);
}
Expand Down
43 changes: 19 additions & 24 deletions src/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import {
CreateClusterResponse,
GetClustersCallback,
GetClustersResponse,
IOperation,
BasicClusterConfig,
} from './cluster';
import {Family} from './family';
import {
Expand All @@ -56,20 +58,15 @@ import {ServiceError} from 'google-gax';
import {Bigtable} from '.';
import {google} from '../protos/protos';

export interface ClusterInfo {
id?: string;
location?: string;
serveNodes?: number;
nodes?: number;
storage?: string;
defaultStorageType?: number;
Comment on lines -59 to -65
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

export interface ClusterInfo extends BasicClusterConfig {
id: string;
}

export interface InstanceOptions {
/**
* The clusters to be created within the instance.
*/
clusters?: ClusterInfo[] | ClusterInfo;
clusters: ClusterInfo[] | ClusterInfo;

/**
* The descriptive name for this instance as it appears in UIs.
Expand Down Expand Up @@ -102,13 +99,16 @@ export interface InstanceOptions {
}

export type IInstance = google.bigtable.admin.v2.IInstance;
export type CreateInstanceCallback = (
err: ServiceError | null,
instance?: Instance,
operation?: Operation,
apiResponse?: IInstance
) => void;
export type CreateInstanceResponse = [Instance, Operation, IInstance];
export interface LongRunningResourceCallback<Resource> {
(
err: ServiceError | null,
resource?: Resource,
operation?: Operation,
apiResponse?: IOperation
): void;
}
export type CreateInstanceCallback = LongRunningResourceCallback<Instance>;
export type CreateInstanceResponse = [Instance, Operation, IOperation];
export type DeleteInstanceCallback = (
err: ServiceError | null,
apiResponse?: google.protobuf.Empty
Expand Down Expand Up @@ -209,9 +209,8 @@ Please use the format 'my-instance' or '${bigtable.projectName}/instances/my-ins
return new AppProfile(this, name);
}

create(options?: InstanceOptions): Promise<CreateInstanceResponse>;
create(options: InstanceOptions): Promise<CreateInstanceResponse>;
create(options: InstanceOptions, callback: CreateInstanceCallback): void;
create(callback: CreateInstanceCallback): void;
/**
* Create an instance.
*
Expand All @@ -231,14 +230,10 @@ Please use the format 'my-instance' or '${bigtable.projectName}/instances/my-ins
* region_tag:bigtable_create_instance
*/
create(
optionsOrCallback?: InstanceOptions | CreateInstanceCallback,
cb?: CreateInstanceCallback
options: InstanceOptions,
callback?: CreateInstanceCallback
): void | Promise<CreateInstanceResponse> {
const options =
typeof optionsOrCallback === 'object' ? optionsOrCallback : {};
const callback =
typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!;
this.bigtable.createInstance(this.id, options, callback);
this.bigtable.createInstance(this.id, options, callback!);
}

createAppProfile(
Expand Down
3 changes: 2 additions & 1 deletion system-test/bigtable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,9 @@ describe('Bigtable', () => {

describe('instances', () => {
it('should get a list of instances', async () => {
const [instances] = await bigtable.getInstances();
const [instances, failedLocations] = await bigtable.getInstances();
assert(instances.length > 0);
assert(Array.isArray(failedLocations));
});

it('should check if an instance exists', async () => {
Expand Down
Loading