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

Variables: Recognize only defined CLI options #9151

Merged
merged 13 commits into from
Mar 22, 2021

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Mar 19, 2021

Not documented part of #8364

With this update only recognized (by CLI command schema) options are resolved out of variables. It's an intended behavior of a new resolver from a beginning. It's a step towards direction, where we abort request if user passes unrecognized options (it's to avoid eventual errors, due to command not being used as intended)

Additionally:

  • Resolve CLI command and options gradually (e.g. do not recognize AWS only commands, when we deal with no AWS provider)
  • Fix resolution of false CLI param values (resolver returned null)
  • Recognize --stage as any (not just AWS) service option. As it's crucial to load.env files, a functionality that's provider agnostic
  • Further improve commands schema, by recognizing --version on each command, and defining help command schema
  • Avoid showing CLI options order deprecation with help request
  • Expose resolved command and command schema on resolveInput result object
  • Reorganize command CLI schema definitions, so they're grouped into three sections (service independent, service dependent, AWS service dependent)
  • Validate provider.name at early stage.

@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #9151 (ea4c2da) into master (6e3e21c) will decrease coverage by 0.09%.
The diff coverage is 82.24%.

❗ Current head ea4c2da differs from pull request most recent head 64f713b. Consider uploading reports for the commit 64f713b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9151      +/-   ##
==========================================
- Coverage   87.01%   86.92%   -0.10%     
==========================================
  Files         293      302       +9     
  Lines       11285    11360      +75     
==========================================
+ Hits         9820     9875      +55     
- Misses       1465     1485      +20     
Impacted Files Coverage Δ
scripts/serverless.js 62.16% <41.17%> (-8.57%) ⬇️
lib/aws/request.js 90.32% <100.00%> (ø)
lib/cli/commands-schema/aws-service.js 100.00% <100.00%> (ø)
.../cli/commands-schema/common-options/aws-service.js 100.00% <100.00%> (ø)
lib/cli/commands-schema/common-options/global.js 100.00% <100.00%> (ø)
lib/cli/commands-schema/common-options/service.js 100.00% <100.00%> (ø)
lib/cli/commands-schema/index.js 100.00% <100.00%> (ø)
lib/cli/commands-schema/no-service.js 100.00% <100.00%> (ø)
lib/cli/commands-schema/service.js 100.00% <100.00%> (ø)
lib/cli/filter-supported-options.js 100.00% <100.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e3e21c...64f713b. Read the comment docs.

@medikoo medikoo requested a review from pgrzesik March 19, 2021 14:42
Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

I have a general comment with commands-schema/aws-service - there's a few of commands defined such as deploy, invoke or invoke local that are not specific to AWS but are also supported e.g. by https://github.com/serverless/serverless-azure-functions. Is that breaking anything for these plugins?


const commands = (module.exports = new Map());

commands.set('deploy', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be only available for aws-service? What about support for e.g. https://github.com/serverless/serverless-azure-functions ?

@medikoo
Copy link
Contributor Author

medikoo commented Mar 22, 2021

I have a general comment with commands-schema/aws-service - there's a few of commands defined such as deploy, invoke or invoke local that are not specific to AWS but are also supported e.g. by https://github.com/serverless/serverless-azure-functions. Is that breaking anything for these plugins?

That's a good point, still let me explain reasoning for such separation.

Commands as configured in aws-service file, are commands which have zero logic configured in core if run in non-AWS provider.

They of course may work in other providers, but given provider is expected to configure some functionality for them, otherwise they will just do nothing.

e.g. in case of Azure, there's a logic configured for sls deploy -> https://github.com/serverless/serverless-azure-functions/blob/c53f535204c467339e8ae1f6edb985c3280bc32c/src/plugins/deploy/azureDeployPlugin.ts#L18

but e.g. Azure does not have any logic defined for sls metrics -> https://github.com/serverless/serverless-azure-functions/tree/master/src/plugins, it's also not existent in its docs. What currently happens is that Framework outputs sls metrics as valid command in help, but when you run it in context of Azure nothing happens.

This PR is first step to make that experience better. All commands configured in aws-service block, will have help displayed for in context of non-AWS provider, only if we find some hooks attached to those command lifecycle events, otherwise we will strip them from help.

Also I plan then to show help for those commands with AWS options stripped (as app or org etc)

Currently this PR doesn't change anything in how help is displayed (it's just internal organization of command schemas that was improved)

@medikoo medikoo requested a review from pgrzesik March 22, 2021 14:08
Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Thanks for extensive clarification - looks great 👍

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.

2 participants