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

Add SLS_NO_WARNINGS env var #6345

Merged
merged 3 commits into from Jul 16, 2019
Merged

Add SLS_NO_WARNINGS env var #6345

merged 3 commits into from Jul 16, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jul 8, 2019

What did you implement:

In logWarning function, added a check for an environment variable named SLS_NO_WARNINGS.

When this env var is set (much like SLS_DEBUG) all the warnings are hidden from the output.

Closes #6346

How did you implement it:

Well, I added a guard clause at the beginning of the logWarning function.

If the env var is set, the function exits without printing.

How can we verify it:

  • In your serverless.yml, try to define some SSM parameters that are not defined
  • Run sls deploy and see the warnings about undefined SSM params
  • Run SLS_NO_WARNINGS=* sls deploy and enjoy no warnings

Todos:

Note: Run npm run test-ci to run all validation checks on proposed changes

  • Write tests and confirm existing functionality is not broken.
    Validate via npm test
  • Write documentation
  • Ensure there are no lint errors.
    Validate via npm run lint-updated
    Note: Some reported issues can be automatically fixed by running npm run lint:fix
  • Ensure introduced changes match Prettier formatting.
    Validate via npm run prettier-check-updated
    Note: All reported issues can be automatically fixed by running npm run prettify-updated
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@ghost ghost mentioned this pull request Jul 8, 2019
@pmuens pmuens self-assigned this Jul 9, 2019
@pmuens pmuens added this to the 1.47.0 milestone Jul 9, 2019
Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Hey @domcorvasce thanks for working on this 👍

I'd propose that we change the environment variable to SLS_WARNINGS so that it sticks to the naming conventions we've established for our ENV vars.

Furthermore we might want to add some documentation for this feature (AFAIK SLS_DEBUG is listed in the --help screen and we're talking about that feature in our docs).

@medikoo what do you think?

@medikoo
Copy link
Contributor

medikoo commented Jul 9, 2019

I'd propose that we change the environment variable to SLS_WARNINGS so that it sticks to the naming conventions we've established for our ENV vars.

As we have warnings on by default, I think SLS_WARNINGS will not work (as by env var boolean case convention it's existence of env var that turns something on). We need to have an env var that turns it off. Still I think better would be SLS_WARNING_DISABLE (it's more on pair with SLS_DEBUG)

@medikoo what do you think?

As a temporary mean, we can take it in, but for a final solution I think it'll be way better to switch to some better logging solution. One which provides us with log levels configurable through env vars out of a box.
In previous projects I successfully relied on log - it's fully customizable on all needed levels, at some good moment we may consider switching to it.

@ghost
Copy link
Author

ghost commented Jul 9, 2019

As we have warnings on by default, I think SLS_WARNINGS will not work (as by env var convention it's existence of env var that turns something on). We need to have an env var that turns it off. Still I think better would be SLS_WARNING_DISABLE (it's more on pair with SLS_DEBUG)

Yeah, it is more self-explanatory that way.

As a temporary mean, we can take it in, but for a final solution I think it'll be way better to switch to some better logging solution. One which provides us with log levels configurable through env vars out of a box.

Absolutely, I agree.

Furthermore we might want to add some documentation for this feature (AFAIK SLS_DEBUG is listed in the --help screen and we're talking about that feature in our docs).

That is the reason why I chose to wait for the documentation. I wasn't sure how to document it properly.

@pmuens pmuens modified the milestones: 1.47.0, 1.48.0 Jul 10, 2019
Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Good thoughts in the thread. I agree with what was mentioned above.

Maybe in the meantime we can take it in and replace it with a proper logging solution later on (which will be a bigger task and takes more time).

@domcorvasce do you want to move forward here? Do you need any help?

@ghost
Copy link
Author

ghost commented Jul 10, 2019

Maybe in the meantime we can take it in and replace it with a proper logging solution later on (which will be a bigger task and takes more time).

@domcorvasce do you want to move forward here? Do you need any help?

Sure, I'm going to push some changes later in the day.

I'll let you know both if I have questions or doubts.

@ghost
Copy link
Author

ghost commented Jul 10, 2019

@pmuens @medikoo Where we should document SLS_WARNING_DISABLE in your opinion?

Maybe I am missing something, but there is no reference to SLS_DEBUG when running:

sls --help

# or

sls deploy --help

Also I wasn't able to find refs within https://serverless.com/framework/docs/ (but I did a quick search, so maybe I am missing something there too).

@pmuens
Copy link
Contributor

pmuens commented Jul 10, 2019

@domcorvasce hmm, yes you're right. It's not show when using serverless --help, however it's displayed when someone runs into an error (see the Error.js class).

🤔 I'm not sure where we should add it. Maybe to the workflow.md docs? Or should we add it to the --help screen? What do you think @medikoo ?

@ghost
Copy link
Author

ghost commented Jul 10, 2019

@domcorvasce hmm, yes you're right. It's not show when using serverless --help, however it's displayed when someone runs into an error (see the Error.js class).

Alright.

🤔 I'm not sure where we should add it. Maybe to the workflow.md docs? Or should we add it to the --help screen? What do you think @medikoo ?

IMO it would be best if we add it to the --help output, which is probably the place where a user is going to look first.

@pmuens
Copy link
Contributor

pmuens commented Jul 10, 2019

IMO it would be best if we add it to the --help output, which is probably the place where a user is going to look first.

Yes, I agree with that. IMHO trying to get this into the doc structure might be too complicated and just adding it to --help might be the easiest option.

@medikoo
Copy link
Contributor

medikoo commented Jul 10, 2019

Maybe to the workflow.md docs? Or should we add it to the --help screen? What do you think @medikoo ?

As I see workflow docs are per provider, not sure if it's a good place, but I think adding note on both SLS_DEBUG and SLS_WARNING_DISABLE to --help will be really helpful for developers. So I'm 👍 for that

@pmuens
Copy link
Contributor

pmuens commented Jul 11, 2019

As I see workflow docs are per provider, not sure if it's a good place, but I think adding note on both SLS_DEBUG and SLS_WARNING_DISABLE to --help will be really helpful for developers. So I'm 👍 for that

👍That sounds reasonable.

Copy link
Contributor

@pmuens pmuens 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 updating @domcorvasce 👍

Just tested it and it looks good! LGTM :shipit:

@pmuens pmuens merged commit 084cee4 into serverless:master Jul 16, 2019
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.

Allow warnings to be hidden
2 participants