Skip to content

Add new method 'defineFunctionProperties' to 'ConfigSchemaHandler' #8462

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 3, 2020
Merged

Add new method 'defineFunctionProperties' to 'ConfigSchemaHandler' #8462

merged 1 commit into from
Nov 3, 2020
+104 −11

Conversation

luislhl
Copy link
Contributor

@luislhl luislhl commented Oct 31, 2020

Closes: #8422

@luislhl
Copy link
Contributor Author

luislhl commented Oct 31, 2020

I tried testing this against Node.js v6, as requested in the PR template, but I was unable to, due to a problem that is also occuring in the master branch.

I thought it would be better to report this here, because I don't know if this is expected. But since it's happening in master, I concluded it's not related to the changes in the PR.

The problem happens when trying to npm install using Node.js v6.17.1 (npm v3.10.10):

> [email protected] postinstall /home/luislhl/Workspace/pessoal/serverless
> node ./scripts/postinstall.js

/home/luislhl/Workspace/pessoal/serverless/node_modules/boxen/index.js:26
			...detail
			^^^

SyntaxError: Unexpected token ...
    at createScript (vm.js:56:10)
    at Object.runInThisContext (vm.js:97:10)
    at Module._compile (module.js:549:28)
    at Object.Module._extensions..js (module.js:586:10)
    at Module.load (module.js:494:32)
    at tryModuleLoad (module.js:453:12)
    at Function.Module._load (module.js:445:3)
    at Module.require (module.js:504:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/luislhl/Workspace/pessoal/serverless/scripts/postinstall.js:3:15)

@luislhl luislhl force-pushed the define-function-properties branch from 8366a3a to 651d96d Compare October 31, 2020 19:59
luislhl
luislhl commented Oct 31, 2020
View reviewed changes
@luislhl luislhl mentioned this pull request Oct 31, 2020
Closed
@fredericbarthelet
Copy link
Contributor

Hi @luislhl and thanks for your PR.

Support for node 6 has been dropped with serverless major v2. The template is not up to date, thanks for pointing this out.

medikoo
medikoo suggested changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@luislhl thank you ! That looks very good. I've proposed just few documentation improvements.

Concerning info about Node.js v6 support, as @fredericbarthelet pointed it was an issue in a template. I've fixed it (it now lists Node.js v10 as expected)

docs/providers/aws/guide/plugins.md Outdated
anotherProperty: { type: 'number' },
},
required: ['someProperty'],
additionalProperties: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not put additionalProperties (it's ineffective), same with type (fact that it's listed in defineCustomProperties example is a bug)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Should I take this opportunity and also remove them from defineCustomProperties?

@luislhl luislhl force-pushed the define-function-properties branch from 651d96d to 15811e3 Compare November 2, 2020 21:05
@luislhl luislhl requested a review from medikoo November 2, 2020 21:09
medikoo
medikoo approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@medikoo medikoo 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 @luislhl That looks great!

@medikoo medikoo merged commit 5003bbf into serverless:master Nov 3, 2020
@luislhl luislhl deleted the define-function-properties branch November 3, 2020 22:58
@DarrenStack DarrenStack mentioned this pull request Dec 11, 2020
Closed
@benjy benjy mentioned this pull request Jul 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@medikoo medikoo medikoo approved these changes

Assignees

@luislhl luislhl

Labels
cat/config-schema enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configuration error at 'functions.hello': unrecognized property 'custom'
3 participants
@luislhl @fredericbarthelet @medikoo