Skip to content

Conversation

joehan
Copy link
Contributor

@joehan joehan commented May 26, 2021

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.

@joehan joehan requested review from taeold and inlined May 26, 2021 18:28
@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label May 26, 2021
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.

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, {
Copy link
Member

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;

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 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.

Copy link
Member

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.

@joehan joehan merged commit a1f2e85 into master May 26, 2021
@joehan joehan deleted the jh-functions-labels branch May 26, 2021 20:47
devpeerapong pushed a commit to devpeerapong/firebase-tools that referenced this pull request Dec 14, 2021
…3408)

* add support for setting user labels on scheduled functions

* Style change to use destructuring, and CHANGELOG entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants