Skip to content

Conversation

taeold
Copy link
Contributor

@taeold taeold commented Mar 2, 2022

Previously, eventFilter attribute was an object:

eventTrigger: {
  eventType: "an.event",
  eventFilter: {
    resource: "my-storage-bucket",
    appId: "12345",
  }
}

We now prefer eventFilter as a list:

eventTrigger: {
  eventType: "an.event",
  eventFilter: [
  {
    attribute: "resource",
    value: "my-storage-bucket",
  }.
  {
    attribute: "appId",
    value: "12345",
  }.
  ]
}

Most of the change in this PR is in tests. Few other minor changes I've squeezed in here:

  1. Added a changelog entry for Add support for more instance sizes and regions #1037
  2. Deleted obsolete src/common/manifest.ts file
  3. Small refactoring of alerts/crashlytics spec to make this migration easier.

@taeold taeold requested review from colerogers and inlined March 2, 2022 16:52
@colerogers
Copy link
Contributor

Seems like we both made a similar set of changes #1047

@taeold
Copy link
Contributor Author

taeold commented Mar 2, 2022

@colerogers D'oh can't believe I missed that! Do you want to update your PR with some of the other minor changes I've made or use this one instead? I didn't look too closely, but it looks like it's mostly char-by-char the same change.

@colerogers
Copy link
Contributor

@taeold We can just use yours! But I found a small issue when I changed the alerts providers, the attributes should not have been camel cased. If you can just yank all the files from v2/providers/alerts/* in my branch to to this branch, we should be good to go!

@taeold
Copy link
Contributor Author

taeold commented Mar 2, 2022

@colerogers Nice! Updated the PR.

@@ -1,0 +1 @@
- Add support for more regions and memory for v2 functions (#1037).
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a changelog entry for this PR too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose not to since this change is refactoring non-user facing APIs. Let me know if you disagree.

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.

Why the empty src/common/manifest.ts?

});
});
});
}
Copy link
Member

Choose a reason for hiding this comment

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

I love the matrix driven tests. Good job deleting so much code!

Simpsons flamethrower meme

@taeold
Copy link
Contributor Author

taeold commented Mar 4, 2022

@inlined It's actually deleted:

https://github.com/firebase/firebase-functions/tree/dl-cf3v2-eventfitlers/src/common

I'm not sure why Github PR displays it as an empty file 🤔

@taeold taeold merged commit f45e45f into master Mar 7, 2022
@taeold taeold deleted the dl-cf3v2-eventfitlers branch March 7, 2022 19:42
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.

3 participants