Skip to content
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

Added eventarc onCustomEventPublished API #1061

Merged
merged 19 commits into from
Apr 6, 2022
Merged

Conversation

pavelgj
Copy link
Collaborator

@pavelgj pavelgj commented Mar 22, 2022

go/rvtfmkswhlks

src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Picked out a few style nits in the comments that get parsed for docgen. Thanks!

src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Sorry, I meant to just comment, not approve (yet). Does this fix that? :)

Copy link
Contributor

@taeold taeold left a comment

Choose a reason for hiding this comment

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

package-lock.json Outdated Show resolved Hide resolved
spec/v2/providers/eventarc.spec.ts Show resolved Hide resolved
src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
v2/eventarc.js Outdated Show resolved Hide resolved
src/v2/providers/eventarc.ts Show resolved Hide resolved
@pavelgj pavelgj force-pushed the pavelj-eventarc-custom branch 2 times, most recently from edf2f82 to 12cbeef Compare April 1, 2022 20:38
@inlined inlined self-requested a review April 5, 2022 17:56
spec/v2/providers/eventarc.spec.ts Outdated Show resolved Hide resolved
spec/v2/providers/eventarc.spec.ts Outdated Show resolved Hide resolved
{
...FULL_OPTIONS,
eventType: 'event-type',
channel: 'locations/us-west1/channels/my-channel',
Copy link
Member

Choose a reason for hiding this comment

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

Per the partial resource name resolution, I think we should also have a test that verifies "my-channel" gets turned into locations/us-central1/channels/my-channel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the functions SDK side I'm only doing the: if channel is not specified, default it to 'locations/us-central1/channels/firebase'. The rest of the partial channel resource name resolution (and validation) is done on the CLI side.

The reason it's done this way is because if channel is not specified, in the container contract I can't distinguish between: it's not specified and expected to be resolved to default vs not specified and should remain undefined.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. And the more code we put in the "agent" (e.g. CLI, control plane) the less we have to redefine in the SDKs.

src/runtime/manifest.ts Outdated Show resolved Hide resolved
src/v2/providers/eventarc.ts Outdated Show resolved Hide resolved
Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

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

Still LGTM. @taeold can you take a pass if necessary and stamp this?

@pavelgj pavelgj merged commit c32db65 into master Apr 6, 2022
@pavelgj pavelgj deleted the pavelj-eventarc-custom branch April 6, 2022 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants