-
Notifications
You must be signed in to change notification settings - Fork 950
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
Add support for setting user labels on scheduled functions #3408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nit
@@ -227,7 +227,9 @@ export function addResourcesToBackend( | |||
cloudFunction.trigger.eventFilters.resource = `${cloudFunction.trigger.eventFilters.resource}/${id}`; | |||
} | |||
|
|||
cloudFunction.labels = { "deployment-scheduled": "true" }; | |||
cloudFunction.labels = Object.assign(cloudFunction.labels, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: now that we have object destructuring I personally find it cleaner to use that syntax:
cloudfunctions.labels = {
...cloudfunctions.lables,
"deployment-scheduled": true,
}
Or at least I would if we wanted to create a copy of the dictionary. In this case, your code is equivalent to:
cloudFunction.labels["deployment-scheduled"] = true;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer that too, so I'll switch this over. As is, this is not quite equivalent to cloudFunction.labels["deployment-scheduled"], because labels is optional & can be undefined. Object.assign safely handles that case, wheres the above does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> obj = {}
{}
> obj.labels = Object.assign(obj.lables, {"foo": "bar"});
Uncaught TypeError: Cannot convert undefined or null to object
at Function.assign (<anonymous>)
Object splat supports undefined but Object.assign
does not.
…3408) * add support for setting user labels on scheduled functions * Style change to use destructuring, and CHANGELOG entry
Description
Adds support for user labels on scheduled functions.
Turns out we already supported user labels in this code path - except that they were being overwritten on scheduled functions.
Scenarios Tested
Confirmed that scheduled functions and non-scheduled functions are deployed with labels when firebase-functions passes them through.