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

Fix: use 'functions' config if no 'functions:functionN' one found #1213

Closed
wants to merge 1 commit into from
Closed

Conversation

yzalvov
Copy link
Contributor

@yzalvov yzalvov commented Apr 23, 2019

Despite the fact that the docs don't cover this yet #the_firebasejson_file, looks like from v6.7.0 one might be able to config firebase.json a level deeper than for a service (individual functions, in my case) so that firebase deploy --only functions:functionN would consider that, but the problem is now the functions level config is ignored if I deploy specific functions (so no lint, no custom scripts).

So please let's use 'functions' config if no 'functions:functionN' one found? It might be deeper than I managed to dig, but this fix works for me.

Description

Bug: firebase.json functions config is ignored if specific functions are deployed.

Scenarios Tested

Before the fix:
Imgur

After the fix:
Imgur

Despite the fact that the docs don't cover this yet https://firebase.google.com/docs/cli/#the_firebasejson_file , looks like from v6.7.0 one might be able to config `firebase.json` a level deeper than a service (individual functions, in my case) so, that `firebase deploy --only functions:functionN` would consider that, but the problem is now the `functions` level config is ignored if I deploy specific functions (so no `lint`, no custom script, you might use).

So please let's use 'functions' config if no 'functions:functionN' one found? It might be deeper than I managed to dig, but this fix works for me.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@coveralls
Copy link

coveralls commented Apr 23, 2019

Coverage Status

Coverage remained the same at 61.557% when pulling 0dc3ce2 on yzalvov:patch-1 into d67ecfa on firebase:master.

@yzalvov
Copy link
Contributor Author

yzalvov commented Apr 23, 2019

📝 Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

I signed it!

@bkendall
Copy link
Contributor

(@yzalvov I think you have to post @googlebot I signed it! without any extra text/quotes in your message)

@yzalvov
Copy link
Contributor Author

yzalvov commented Apr 23, 2019

@googlebot I signed it!

@Memeriaj
Copy link
Contributor

I think that this should be fine. As it stands now you can only have lifecycle hooks for each resource or for the product overall. This is because the config will either be an object (if there is only a single resource) or an array (if there are many resources). Functions is a little bit weird in that you can deploy each one individually but the config is set up for the entire product (which is why I didn't realize that the lifecycle hooks would act strangely for it).

Let me go find one of my old PRs for this repo where the googlebot didn't think I was part of the organization and see what I needed to say to it.

Copy link
Contributor

@Memeriaj Memeriaj left a comment

Choose a reason for hiding this comment

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

Wait, we should add a changelog entry. It should be something like fixed - Functions lifecycle hooks should trigger for individual deploys

@Memeriaj
Copy link
Contributor

You need to leave a comment with just I signed it! I believe for the CLA thing to work properly. Or at least I have evidence from #1047 that it works. I have no idea why the regex isn't a little smarter.

@yzalvov yzalvov mentioned this pull request Apr 23, 2019
@yzalvov
Copy link
Contributor Author

yzalvov commented Apr 23, 2019

I signed it!

@Memeriaj
Copy link
Contributor

Arg, let me investigate the CLA stuff.

@Memeriaj
Copy link
Contributor

Alright I figured out the CLA problem, the commit has 25827414+yzalvov@users.noreply.github.com as the author. I assume that this is from a privacy setting in your GitHub profile. We can

  1. Have you rebase your change so that it uses your primary GitHub email address
  2. I can create a new PR with your changes, referencing them, and send you the review

@Memeriaj
Copy link
Contributor

@yzalvov take a look at #1216 just double check that it contains your changes. If you'd rather adjust these PRs (or even create a new one yourself) that is perfectly fine, I just wanted to make this as easy for you as I can.

@yzalvov
Copy link
Contributor Author

yzalvov commented Apr 24, 2019

@Memeriaj thank you. I like your #1216 title better, but please let me finish this 'contribution quest' by myself :) So please check #1217. I guess now it should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants