-
Notifications
You must be signed in to change notification settings - Fork 62
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
chore: add JSON schema for netlify.toml
#4585
base: main
Are you sure you want to change the base?
Conversation
@danez any updates on this one? Would love having autocompletion for the toml 😂 |
netlify.toml
18e199e
to
6847f55
Compare
98eb976
to
4a88be6
Compare
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.
Hey @danez, sorry for the delay in reviewing this.
I know you copied all of these from the docs but I left a number of suggested edits to make some items more concise, clearer, or to just follow a more consistent format. Hopefully they should be quick and easy to apply but let me know if you have questions.
Since there are a bunch of edits, I'll go ahead and request changes here. But I'll keep an eye out to re-review this once you're ready.
|
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.
Hey @danez thanks for making these changes!
In this review, I spotted one typo that I missed last time and I tagged @klavavej to review the caching updates as I know the docs for EF caching are still a WIP. I just want to make sure we use the same language. I did leave some suggestions in the meantime.
I also checked all your links this time and can confirm they work as expected 👍
I don't see any blockers, so I'll approve. But it might be worth waiting for Kristen's approval too.
"definitions": { | ||
"BasePath": { | ||
"title": "Base path", | ||
"description": "Directory to change to before starting a build. This is where we will look dependency management files such as `package.json` or `.nvmrc`. If not set, defaults to the root directory.", |
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.
Ah, I must have made a typo in my suggestion, sorry. We're missing a word here
"description": "Directory to change to before starting a build. This is where we will look dependency management files such as `package.json` or `.nvmrc`. If not set, defaults to the root directory.", | |
"description": "Directory to change to before starting a build. This is where we will look for dependency management files such as `package.json` or `.nvmrc`. If not set, defaults to the root directory.", |
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.
Thanks for the ping @codebyuma ! I left a few notes but nothing blocking from me either so I will approve
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 think the template
field is missing on the root and the functions
field on the build settings.
[build]
# This is where we will look for your lambda functions
functions = "project/functions/"
and for example:
# Set enviroment variable prompts for templates
[template.environment]
YOUR_ENV_KEYS_NEEDED = "Enter in the ENV key here"
# ref https://bit.ly/2wQ1mVk
incoming-hooks = ["Service-1"]
The docs though say it should be this: [functions]
directory = "my_functions" maybe |
mhm maybe @eduardoboucas can put some light on that? |
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.
Left one small suggested edit but otherwise the latest updates look good to me 🤞
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.
Not sure if you still need another technical review but the copy looks good to me 🚢
22d3967
to
29e80c2
Compare
Co-authored-by: Uma Chandran <[email protected]>
The config for edge functions is currently incorrect in the schema:
|
|
Summary
This PR adds a new JSON schema file for netlify.toml. A direct link to this file will be added to https://github.com/SchemaStore/schemastore. Most IDEs (vscode with extension, intellij) try to read schemas from this store.
In the future, the schema could even be used to validate the
netlify.toml
file in@netlify/config
, as I saw right now we have some hardcoded validation in the code.The new package is private for now, so even if we want to rename it later into maybe
config-validation
we can simply do this.I added docs as reviewer, because I added a lot of links pointing to the docs and also copied most of the text from the docs.
Ref: https://github.com/netlify/pod-compute/issues/138
A picture of a cute animal (not mandatory, but encouraged)