GCS Enhancement #981

colerogers · 2021-09-23T18:10:08Z

Description

Enhancement to how customers use GCS Triggered Functions.

Code sample

onObjectArchived()
onObjectFinalized()
onObjectDeleted()
onObjectMetadataUpdated()

…ncement

taeold

Minor nits. Hoping we can find a way to reduce some repetition in code.

taeold · 2021-09-24T16:57:18Z

src/v2/providers/storage.ts

+/** @hidden */
+export const provider = 'google.cloud.storage';
+/** @hidden */
+export const service = 'storage.googleapis.com';
+/** @hidden */
+export const archivedEvent = 'object.v1.archived';
+/** @hidden */
+export const finalizedEvent = 'object.v1.finalized';
+/** @hidden */
+export const deletedEvent = 'object.v1.deleted';
+/** @hidden */
+export const metadataUpdatedEvent = 'object.v1.metadataUpdated';


Do we need to export these constants?

I'm using them in the unit tests

taeold · 2021-09-24T17:11:55Z

src/v2/providers/storage.ts

+  if (arguments.length === 1) {
+    handler = optsOrHandler as (
+      event: CloudEvent<StorageObjectData>
+    ) => any | Promise<any>;
+    _onOperation(handler, archivedEvent);
+  }
+  return _onOperation(
+    handler,
+    archivedEvent,
+    optsOrHandler as string | StorageOptions
+  );
+}


Hmmm, it looks like we repeat this block for each of the four event type. It would be cool if we can contain all "make sure we handle optOrHandler appropriately in _onOperation; can we start a discussion in our chat room to see if there's a clever TS thing we can do?

I've cleaned it up and moved the logic into _onOperation. The 4 functions now only serve to inject the correct event type.

taeold · 2021-09-24T17:20:11Z

src/v2/providers/storage.ts

+ * An object within Google Cloud Storage.
+ * Ref: https://github.com/googleapis/google-cloudevents-nodejs/blob/main/cloud/storage/v1/StorageObjectData.ts
+ */
+export interface StorageObjectData {


nit* Can this be declared at the top of the file instead?

Moved to the top

taeold · 2021-09-24T17:25:49Z

src/v2/providers/storage.ts

+        },
+        eventTrigger: {
+          eventType: provider + '.' + eventType,
+          resource: bucket, // TODO(colerogers): replace with bucket: 'my-bucket' when container contract is finished


Can you expand on what you mean by this TODO? I'm a confused by the suggestion that we should set this to a constant my-bucket. Also probably better to leave out mentioning container contract here.

Good catch, I removed the CC part. Eventually, we'll need to remove the resource field and just map to a bucket field.

taeold · 2021-09-24T17:28:23Z

src/v2/providers/storage.ts

+      'Missing bucket name. If you are unit testing, please provide a bucket name' +
+        ' through set process.env.FIREBASE_CONFIG.'


Suggested change

'Missing bucket name. If you are unit testing, please provide a bucket name' +

' through set process.env.FIREBASE_CONFIG.'

'Missing bucket name. If you are unit testing, please provide a bucket name' +

' by providing bucket name directly in the event handler or by setting process.env.FIREBASE_CONFIG.'

);

taeold · 2021-09-24T17:29:09Z

src/v2/providers/storage.ts

+): CloudFunction<StorageObjectData>;
+
+export function onObjectMetadataUpdated(
+  optsOrHandler:


nits* It's a mouthful, but I think I would prefer butcketOrOptsOrHandler for accuracy.

Makes sense, fixed

taeold · 2021-09-24T17:32:02Z

spec/v2/providers/storage.spec.ts

+    };
+
+    beforeEach(() => {
+      options.setGlobalOptions({});


This seems redundant no?

taeold · 2021-09-24T17:33:19Z

spec/v2/providers/storage.spec.ts

+
+    beforeEach(() => {
+      options.setGlobalOptions({});
+      process.env.GCLOUD_PROJECT = 'aProject';


Hmm can you remind me why setting this environment variable is required for the test to work?

If we don't set it, then when we try and perform optionsToTriggerAnnotations it will call serviceAccountFromShorthand which requires this env var to be set and throw an error otherwise

taeold · 2021-09-24T17:35:56Z

spec/v2/providers/storage.spec.ts

+    const EVENT_TRIGGER = {
+      eventType: 'google.cloud.storage.event-type',
+      resource: 'my-bucket',
+      service: 'storage.googleapis.com',
+    };


nit* I'd make this module-level constant instead of scoping it to this test.

Moved to the top and added in specific event constants for each event type.

taeold · 2021-09-24T17:37:56Z

spec/v2/providers/storage.spec.ts

+        },
+        regions: ['us-west1'],
+      });
+    });


Can we also test where opt is provided without a bucket property?

Added tests for each event type tests and the onOperation tests.

taeold · 2021-09-24T21:43:54Z

src/v2/providers/storage.ts

+  handler: (event: CloudEvent<StorageObjectData>) => any | Promise<any>
+): CloudFunction<StorageObjectData>;
+
+/** Handle a storage object archived */


ugh does ESLint for us to repeat JSDoc comment on each of the overloaded function 🤦‍♂️ ?

I was following what we did with pubsub, but since GCS has a lot more overloads, I removed the repeated ones and left the top comment.

taeold · 2021-09-24T21:44:33Z

src/v2/providers/storage.ts

+  // handler: (event: CloudEvent<StorageObjectData>) => any | Promise<any>,
+  // eventType: string,
+  // bucketOrOpts?: string | StorageOptions


Suggested change

// handler: (event: CloudEvent<StorageObjectData>) => any | Promise<any>,

// eventType: string,

// bucketOrOpts?: string | StorageOptions

Thanks, removed

taeold · 2021-09-24T21:46:42Z

src/v2/providers/storage.ts

+    | ((event: CloudEvent<StorageObjectData>) => any | Promise<any>),
+  handler?: (event: CloudEvent<StorageObjectData>) => any | Promise<any>
+): CloudFunction<StorageObjectData> {
+  if (arguments.length === 2) {


Isn't there a subtle bug here since _onOperation is always called with 3 arguments now?

e.g.

> function foo(a, b) { return arguments.length } undefined > foo(1) 1 > foo(1, 2) 2 > foo(1, undefined) 2

So I'd expect arguments.length to always be 3

Maybe this will work:

if (typeof bucketOrOptsOrHandler === "function") { ... }

Oh dang, yeah that's a bug alright. For some reason in my head passing an undefined handler was equivalent to leaving off the last parameter. Thanks for catching this!

taeold

lgtm; I'd recommend we wait until @inlined review as this is our first major enhancement on v2 provider namespace outside of his code.

inlined

Sorry... I reviewed this a while ago and never hit "submit"

package.json

spec/v2/providers/storage.spec.ts

src/v2/providers/storage.ts

inlined · 2021-09-27T19:24:01Z

src/v2/providers/storage.ts

+}
+
+/** @hidden */
+export function _getOptsAndBucket(


While you'll still find _ prefixes in old codebases, that was pre access-control. Also, I think you can use @internal instead of @hidden (would need to double check that it doesn't show up in docs).

Removed the _ prefixes and changed to using @internal. The typedoc docs indicate that both hidden and internal will remove from the code from documentation as long as we don't pass in --excludeInternal.

colerogers added 4 commits September 17, 2021 14:28

inital gcs work

ba1c575

removing bucket path

55da612

Merge remote-tracking branch 'origin/master' into colerogers.gcs-enha…

49a785e

…ncement

adding tests

bcfd298

colerogers requested a review from inlined September 23, 2021 18:10

google-cla bot added the cla: yes label Sep 23, 2021

colerogers requested a review from taeold September 23, 2021 18:12

colerogers mentioned this pull request Sep 23, 2021

GCS Enhancement firebase/firebase-tools#3776

Merged

taeold reviewed Sep 24, 2021

View reviewed changes

colerogers added 3 commits September 24, 2021 14:03

cleaning up and fixing pr issues

02db134

clean up comment

3232256

removing redundant code

2833c3f

taeold reviewed Sep 24, 2021

View reviewed changes

colerogers added 2 commits September 24, 2021 15:45

fixing undefined argument bug

16db16c

format:fix

3b04291

taeold approved these changes Sep 25, 2021

View reviewed changes

inlined approved these changes Sep 27, 2021

View reviewed changes

colerogers added 3 commits September 29, 2021 09:48

removing apiVersion, changing hidden to internal, and cleaning up tests

2c51eba

format

30e116b

changelog

fa412ce

colerogers merged commit 0479817 into master Sep 30, 2021

colerogers deleted the colerogers.gcs-enhancement branch September 30, 2021 18:19

colerogers added the perf-h1-2022 label Feb 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCS Enhancement #981

GCS Enhancement #981

colerogers commented Sep 23, 2021

taeold left a comment

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold Sep 24, 2021

taeold Sep 24, 2021

colerogers Sep 24, 2021

taeold left a comment

inlined left a comment

inlined Sep 27, 2021

colerogers Sep 29, 2021

		'Missing bucket name. If you are unit testing, please provide a bucket name' +
		' through set process.env.FIREBASE_CONFIG.'

	// handler: (event: CloudEvent<StorageObjectData>) => any \| Promise<any>,
	// eventType: string,
	// bucketOrOpts?: string \| StorageOptions

GCS Enhancement #981

GCS Enhancement #981

Conversation

colerogers commented Sep 23, 2021

Description

Code sample

taeold 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

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

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

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

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

taeold left a comment

Choose a reason for hiding this comment

inlined left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment