Skip to content

Conversation

@jthegedus
Copy link
Contributor

@jthegedus jthegedus commented Jun 25, 2021

Description

Expand the JSON Schema & Firebase Config type definition as discussed in #1409 (comment)

The large diff is because the final .json schema is generated.

I am not a daily TS user so there may be better ways of doing some of this.

deploy hooks with hosting:

I am unsure of predeploy/postdeploy hooks when referencing arrays of resources, ie: when hosting is an array:

{
	hosting: [{
		predeploy: "",
		postdeploy: "",
	}, {
		predeploy: "",
		postdeploy: "",
	}]
}

This seems logical as they cannot appear under the root of hosting as it's no longer an object, but I have never seen anyone actually use pre/post hooks in this type of a config.

Side note, I really wish the object or array of objects ({...} | [{...}]) stuff didn't exist. As with the source | regex in Hosting config. It makes validation much more difficult and the output config very large.

Scenarios Tested

NA

Sample Commands

NA

@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label Jun 25, 2021
Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on! I have some comments. A lot of annoying naming nitpicks but I want to make sure this file is maintainable.

@jthegedus
Copy link
Contributor Author

Updated. I would urge in future we source the appropriate runtime & region values from elsewhere in this codebase as they will need to be hardcoded somewhere here surely?

@samtstern
Copy link
Contributor

samtstern commented Jul 1, 2021

cc @joehan @inlined @taeold do we have a canonical list of Functions / Cloud Run regions in the codebase anywhere?

@jthegedus thanks for making those changes! LGTM now. I actually don't think we have a canonical region list in the codebase anywhere but I think you're right that it would be smart to have a constant list somewhere and share it among different parts of the codebase. I have a feeling the omission was intentional though, so hoping someone else can comment.

@samtstern samtstern merged commit a25a88b into firebase:master Jul 1, 2021
@taeold
Copy link
Contributor

taeold commented Jul 1, 2021

@samtstern Found this in firebase-functions repo, but don't see something in this repo.

I do think the omission is intentional though - based on this comment, what we prefer is to query the GCF backend to get the list of supported region during deploys instead of hard-coding the list in the codebase.

(tangent: I learned that the CLI never actually validates whether the region is valid. It just fails in the process because some URL we create w/ the user-provided region points to a non-existent endpoint 🤷 )

@samtstern
Copy link
Contributor

@taeold I think that makes sense, this way we don't have to chase the regions list and update firebase-tools all the time. It also means that users of old CLI versions can deploy to new GCF regions, which is often useful. Thanks for confirming!

@jthegedus
Copy link
Contributor Author

It would be cool if we could fetch the external list in the schema file 🤔

@jthegedus jthegedus deleted the feat/expand-firebase-config branch July 2, 2021 08:11
@jthegedus
Copy link
Contributor Author

(tangent: I learned that the CLI never actually validates whether the region is valid. It just fails in the process because some URL we create w/ the user-provided region points to a non-existent endpoint shrug )

It should probably validate before say a joint resource deployment puts the environment in a partially deployed state (I haven't checked the codebase yet to see if this is already catered to)

@taeold
Copy link
Contributor

taeold commented Jul 3, 2021

@jthegedus I think the deployment fails early enough to not put deployment states in a partial state.

The deployment fails late enough to have wasted time uploading user code and with somewhat hard-to-spot error message:

exports.helloWorld = functions
    .region("hello")
    .https.onRequest((request, response) => {
      response.send("Hello from Firebase!");
    });
 % firebase deploy --only functions
✔  functions: Finished running predeploy script.
i  functions: ensuring required API cloudfunctions.googleapis.com is enabled...
i  functions: ensuring required API cloudbuild.googleapis.com is enabled...
✔  functions: required API cloudbuild.googleapis.com is enabled
✔  functions: required API cloudfunctions.googleapis.com is enabled
i  functions: preparing functions directory for uploading...
i  functions: packaged functions (68.65 KB) for uploading
✔  functions: functions folder uploaded successfully
i  functions: creating Node.js 12 function helloWorld(hello)...
⚠  functions: failed to create function projects/---/locations/hello/functions/helloWorld
i  functions: cleaning up build files...

Functions deploy had errors with the following functions:
	helloWorld(hello)

To try redeploying those functions, run:
    firebase deploy --only "functions:helloWorld"

To continue deploying other features (such as database), run:
    firebase deploy --except functions

Error: Functions did not deploy properly.

Pretty late in the deployment without clear error message :(. It's only obvious if you use --debug flag.

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.

3 participants