Skip to content

Conversation

blidd-google
Copy link
Contributor

@blidd-google blidd-google commented Nov 8, 2022

Some customers were experiencing deployment failures while attempting to deploy custom event triggered functions using the onCustomEventPublished providers (#5153).

Our hypothesis is that the customer was attempting to deploy the custom event function before creating the Eventarc channel on which the function would be listening for events, and as a result, GCF threw an error back at the customer.

This PR patches the missing Eventarc channel issue by:

  1. detecting which channel the function was expecting to listen on,
  2. checking if the channel already exists,
  3. if it doesn't exist, creating the new channel via the Eventarc API, and
  4. deploying the function.

@blidd-google blidd-google requested a review from inlined November 8, 2022 22:19
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.

I'm sure the channel didn't come with a project ID from the container contract. Do we need to normalize the channel that we get from functions.yaml into a full resource name? If so, where does that happen?

operationResourceName: op.name,
});
} catch (err: any) {
if (err.status === 409) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment that we get a CONFLICT because the channel has been created already and therefore we don't need to worry about the channel already existing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and yes agreed about adding a comment

* Creates a channel.
*/
export async function createChannel(channel: Channel): Promise<Operation> {
const body: Partial<Channel> = cloneDeep(channel);
Copy link
Member

Choose a reason for hiding this comment

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

We needed to clone into a Partial<Channel> because we previously deleted the name. Now that we're not doing this we don't need to clone the body or use partial types.

);
});

it.only("handles already existing eventarc channels", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

What does it.only do rather than it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I forgot to take that out, hadn't cleaned up the PR before creating the draft!

If some unit tests are specified with it.only, only they will run when the npm run test command is executed.

@blidd-google blidd-google marked this pull request as ready for review November 9, 2022 20:29
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.

Some nits for you, thanks for sending!

@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2022

Codecov Report

Base: 56.35% // Head: 56.37% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (45aac47) compared to base (270f419).
Patch coverage: 69.69% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5226      +/-   ##
==========================================
+ Coverage   56.35%   56.37%   +0.02%     
==========================================
  Files         312      313       +1     
  Lines       21017    21049      +32     
  Branches     4271     4275       +4     
==========================================
+ Hits        11844    11867      +23     
- Misses       8147     8157      +10     
+ Partials     1026     1025       -1     
Impacted Files Coverage Δ
src/deploy/functions/release/reporter.ts 91.05% <ø> (ø)
src/gcp/eventarc.ts 50.00% <50.00%> (ø)
src/api.ts 95.38% <100.00%> (+0.07%) ⬆️
src/deploy/functions/release/fabricator.ts 84.14% <100.00%> (+0.55%) ⬆️
src/gcp/cloudfunctionsv2.ts 62.63% <0.00%> (+0.52%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

LGTM, thanks!

@blidd-google blidd-google enabled auto-merge (squash) November 22, 2022 17:15
@blidd-google blidd-google merged commit 891c2f8 into master Dec 13, 2022
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.

4 participants