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

Add support for setting user labels on scheduled functions #3408

Merged
merged 4 commits into from
May 26, 2021

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