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

feat: added configurable warning limits through .thresholdrc file #132

Merged
merged 1 commit into from
Feb 10, 2020

Conversation

barrett-schonefeld
Copy link
Contributor

@barrett-schonefeld barrett-schonefeld commented Feb 6, 2020

Changes:

  • added code to process a .thresholdrc file to get warnings limit
  • used warning limit to set exit code to 1 and print a message if warning limit is exceeded
  • added validation on the user input from .thresholdrc to check if valid keys and values provided in .thresholdrc
  • set default limit to Number.MAX_VALUE if no .thresholdrc provided
  • added tests to test error handling for .thresholdrc file and expected output when warning limits exceeded
  • refactored error printing in processConfiguration file into a printConfigErrors function

Current user contract:

  • User must have .thresholdrc file in the root of the their project (if Validator part of automated build) or must be in CWD if running Validator manually with lint-openapi command.
  • Warning limit set to max value by default. .thresholdrc file must be used to add warnings limit.

Example output:

Error handling in .thresholdrc file

Screen Shot 2020-02-06 at 11 05 38 AM

Warning limit exceeded message

Screen Shot 2020-02-06 at 11 08 53 AM

@barrett-schonefeld barrett-schonefeld changed the title wip: added configurable warning limits through .thresholdrc file feat: added configurable warning limits through .thresholdrc file Feb 6, 2020
@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@09315b4). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #132   +/-   ##
=========================================
  Coverage          ?   92.84%           
=========================================
  Files             ?       56           
  Lines             ?     2109           
  Branches          ?        0           
=========================================
  Hits              ?     1958           
  Misses            ?      151           
  Partials          ?        0
Impacted Files Coverage Δ
src/cli-validator/runValidator.js 93.93% <100%> (ø)
src/cli-validator/utils/processConfiguration.js 90.85% <100%> (ø)

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 09315b4...21edbfc. Read the comment docs.

@mkistler
Copy link
Contributor

mkistler commented Feb 7, 2020

Added @danhudlow to evaluate if this solution works for VPC.

Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

This looks good but you need to add documentation for .thresholdrc to the README.

Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Great work on this PR! I do have a number of changes to make before merging but the overall changes look good

README.md Show resolved Hide resolved
src/cli-validator/utils/processConfiguration.js Outdated Show resolved Hide resolved
test/cli-validator/mockFiles/thresholds/.zeroWarnings Outdated Show resolved Hide resolved
test/cli-validator/tests/thresholdValidator.js Outdated Show resolved Hide resolved
test/cli-validator/tests/thresholdValidator.js Outdated Show resolved Hide resolved
test/cli-validator/tests/thresholdValidator.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

- added code to process a .thresholdrc file to get warnings limit
- used warning limit to set exit code to 1 and print a message if warning limit is exceeded
- added validation on the user input from .thresholdrc to check if valid keys and values provided in .thresholdrc
- set default limit to Number.MAX_VALUE if no .thresholdrc provided
- added tests to test error handling for .thresholdrc file and expected output when warning limits exceeded
- added test to check if error thrown when invalid JSON provided in .thresholdrc
- refactored error printing in processConfiguration file into a printConfigErrors function
- ran npm run fix to fix style
- documented how to add a warnings limit in the README
Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good - nice work! 👍

@dpopp07 dpopp07 merged commit 2c3a919 into master Feb 10, 2020
dpopp07 pushed a commit that referenced this pull request Feb 10, 2020
# [0.19.0](v0.18.0...v0.19.0) (2020-02-10)

### Features

* added configurable warning limits through .thresholdrc file ([#132](#132)) ([2c3a919](2c3a919))
@dpopp07
Copy link
Member

dpopp07 commented Feb 10, 2020

🎉 This PR is included in version 0.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dpopp07 dpopp07 deleted the limits branch February 10, 2020 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants