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.

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

feat: Better error messages when config validation fails #768

Merged
merged 39 commits into from
Nov 17, 2023
Merged

feat: Better error messages when config validation fails #768

merged 39 commits into from
Nov 17, 2023

Conversation

edgarrmondragon
Copy link
Collaborator

@edgarrmondragon edgarrmondragon commented Jun 30, 2022 •
edited
Loading

Summary of changes

  1. Writes each validation error message to stderr and aborts the app execution:

    $ poetry run python samples/sample_tap_sqlite
2023-11-17 16:09:23,020 | ERROR    | tap-sqlite-sample    | Config validation error: 'path_to_db' is a required property

  2. Adds tests that validate the tap and target constructors and CLI output.

@edgarrmondragon edgarrmondragon self-assigned this Jun 30, 2022
@edgarrmondragon edgarrmondragon force-pushed the 753-validate-taptarget-config-options branch from f1cf952 to 3b0750a Compare June 30, 2022 02:29
@edgarrmondragon edgarrmondragon force-pushed the 753-validate-taptarget-config-options branch from 3b0750a to 108a87c Compare June 30, 2022 02:40
@codecov
Copy link

codecov bot commented Jun 30, 2022 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b8ab97e) 87.00% compared to head (93e29c0) 87.45%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #768      +/-   ##
==========================================
+ Coverage   87.00%   87.45%   +0.44%     
==========================================
  Files          58       58              
  Lines        4855     4861       +6     
  Branches      764      763       -1     
==========================================
+ Hits         4224     4251      +27     
+ Misses        446      429      -17     
+ Partials      185      181       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@edgarrmondragon edgarrmondragon force-pushed the 753-validate-taptarget-config-options branch 2 times, most recently from b1d234b to 9e52cd8 Compare August 9, 2022 21:54
@edgarrmondragon edgarrmondragon force-pushed the 753-validate-taptarget-config-options branch from 9e52cd8 to c05087e Compare August 9, 2022 22:01
@edgarrmondragon edgarrmondragon marked this pull request as ready for review August 9, 2022 22:12
@edgarrmondragon edgarrmondragon force-pushed the 753-validate-taptarget-config-options branch from bcbac72 to 39af208 Compare August 23, 2022 15:53
@edgarrmondragon edgarrmondragon force-pushed the 753-validate-taptarget-config-options branch from 32b1706 to 7e1d10a Compare August 23, 2022 16:27
@edgarrmondragon edgarrmondragon changed the title feat: Validate unsupported config options feat: Support validating unsupported config options Aug 24, 2022
@edgarrmondragon edgarrmondragon changed the title feat: Support validating unsupported config options feat: Support validating unknown config options Aug 24, 2022
@edgarrmondragon edgarrmondragon force-pushed the 753-validate-taptarget-config-options branch from 115c92a to ee039fc Compare February 16, 2023 02:16
@edgarrmondragon edgarrmondragon mentioned this pull request Feb 24, 2023
Merged
@edgarrmondragon edgarrmondragon mentioned this pull request Mar 24, 2023
Merged
laurentS
laurentS approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@laurentS laurentS left a comment

Choose a reason for hiding this comment

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

This seems very helpful!

singer_sdk/exceptions.py Show resolved Hide resolved
@edgarrmondragon edgarrmondragon mentioned this pull request Jul 18, 2023
Merged
@edgarrmondragon edgarrmondragon removed the request for review from aaronsteers July 18, 2023 23:01
@edgarrmondragon edgarrmondragon mentioned this pull request Jul 19, 2023
Closed
@edgarrmondragon edgarrmondragon removed the request for review from cjohnhanson July 20, 2023 20:15
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Nov 17, 2023 •
edited
Loading

CodSpeed Performance Report

Merging #768 will not alter performance

Comparing 753-validate-taptarget-config-options (93e29c0) with main (b8ab97e)

Summary

✅ 1 untouched benchmarks

@edgarrmondragon edgarrmondragon force-pushed the 753-validate-taptarget-config-options branch from 383ec52 to 93e29c0 Compare November 17, 2023 21:57
@edgarrmondragon edgarrmondragon merged commit 2a64cf6 into main Nov 17, 2023
30 checks passed
@edgarrmondragon edgarrmondragon deleted the 753-validate-taptarget-config-options branch November 17, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurentS laurentS laurentS approved these changes

@WillDaSilva WillDaSilva WillDaSilva left review comments

@kgpayne kgpayne Awaiting requested review from kgpayne

Assignees

@edgarrmondragon edgarrmondragon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edgarrmondragon @laurentS @WillDaSilva