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

11034 extend configuration #11558

Merged
merged 36 commits into from Dec 20, 2022
Merged

Conversation

mklenbw
Copy link
Contributor

@mklenbw mklenbw commented Nov 28, 2022

Adds a new method in serverless-object for extending the internal configuration. This method extends variablesMeta with the newly added and parsed configuration.

VariablesMeta is now exposed via serverless-object.

Addresses the issue discussed in #11034.

Closes: #11034

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.

@mklenbw thank you, that looks outstanding! We're on a very good track, please see my comments

lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
scripts/serverless.js Outdated Show resolved Hide resolved
test/unit/lib/serverless.test.js Outdated Show resolved Hide resolved
@mklenbw mklenbw requested a review from medikoo December 2, 2022 10:06
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.

@mklenbw great thanks for the update!

lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
test/unit/lib/serverless.test.js Outdated Show resolved Hide resolved
test/unit/lib/serverless.test.js Outdated Show resolved Hide resolved
@mklenbw
Copy link
Contributor Author

mklenbw commented Dec 12, 2022

@medikoo
Got the fixtures in place. It's slow, complex and hard to debug, but it works.
Managed to find some errors in script/serverless this way. Wasn't the worst idea to do this.

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.

@mklenbw that looks really good! We're getting close. Please see my comments

lib/serverless.js Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
lib/serverless.js Outdated Show resolved Hide resolved
scripts/serverless.js Outdated Show resolved Hide resolved
test/unit/lib/serverless.test.js Outdated Show resolved Hide resolved
@mklenbw
Copy link
Contributor Author

mklenbw commented Dec 13, 2022

@medikoo
please recheck your comments.

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 @mklenbw!

scripts/serverless.js Outdated Show resolved Hide resolved
test/unit/lib/serverless.test.js Outdated Show resolved Hide resolved
test/unit/lib/serverless.test.js Outdated Show resolved Hide resolved
test/unit/lib/serverless.test.js Outdated Show resolved Hide resolved
test/unit/lib/serverless.test.js Outdated Show resolved Hide resolved
scripts/serverless.js Show resolved Hide resolved
@mklenbw
Copy link
Contributor Author

mklenbw commented Dec 15, 2022

@medikoo
Done with your comments. Please review again.

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.

@mklenbw that looks really good.

What we miss is documentation. Can you add a Extending configuration section here: https://github.com/serverless/serverless/tree/main/docs/guides/plugins and document shortly the new feature?

@mklenbw
Copy link
Contributor Author

mklenbw commented Dec 15, 2022

@mklenbw that looks really good.

What we miss is documentation. Can you add a Extending configuration section here: https://github.com/serverless/serverless/tree/main/docs/guides/plugins and document shortly the new feature?

There is already a section named "Extending the configuration". How do you want it to be named?

@medikoo
Copy link
Contributor

medikoo commented Dec 15, 2022

There is already a section named "Extending the configuration". How do you want it to be named?

@mklenbw that is a good point. Actually, I think we should rename the current "Extending the configuration" section into "Extending the configuration schema", and add new section "Extending and overriding configuration".

@mklenbw
Copy link
Contributor Author

mklenbw commented Dec 15, 2022

@medikoo
please review the documentation page. i don't know if you use any tool for this as there are autogenerated parts in each file

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.

@mklenbw sorry for late review. That looks great! I have just final suggestion for documentation and we should be good

docs/guides/plugins/extending-configuration.md Outdated Show resolved Hide resolved
@mklenbw
Copy link
Contributor Author

mklenbw commented Dec 20, 2022

@mklenbw sorry for late review. That looks great! I have just final suggestion for documentation and we should be good

Done. Thanks. Looking forward to use this feature in official version.

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 @mklenbw That's outstanding contribution!

@medikoo medikoo merged commit 968ddd5 into serverless:main Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolver does not reload variable meta
2 participants