Skip to content

Conversation

@bbernays
Copy link
Contributor

@bbernays bbernays commented Feb 1, 2023

Summary

Enable each individual plugin to pass in any and all additional validator functions they want...


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@github-actions
Copy link

github-actions bot commented Feb 1, 2023

⏱️ Benchmark results

Comparing with 5aebba3

  • DefaultConcurrencyDFS-2 resources/s: 10,185 ⬇️ 9.82% decrease vs. 5aebba3
  • DefaultConcurrencyRoundRobin-2 resources/s: 11,147 ⬇️ 0.04% decrease vs. 5aebba3
  • Glob-2 ns/op: 150.6 ⬇️ 12.28% decrease vs. 5aebba3
  • TablesWithChildrenDFS-2 resources/s: 28,512 ⬇️ 1.16% decrease vs. 5aebba3
  • TablesWithChildrenRoundRobin-2 resources/s: 28,929 ⬇️ 2.45% decrease vs. 5aebba3
  • TablesWithRateLimitingDFS-2 resources/s: 28.18 ⬇️ 0.85% decrease vs. 5aebba3
  • TablesWithRateLimitingRoundRobin-2 resources/s: 828.4 ⬆️ 2.51% increase vs. 5aebba3
  • BufferedScanner-2 ns/op: 9.283 ⬇️ 0.20% decrease vs. 5aebba3
  • LogReader-2 ns/op: 31.69 ⬆️ 1.74% increase vs. 5aebba3

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2023

Codecov Report

Base: 47.16% // Head: 47.06% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (5902ff1) compared to base (5aebba3).
Patch coverage: 0.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #654      +/-   ##
==========================================
- Coverage   47.16%   47.06%   -0.10%     
==========================================
  Files          70       70              
  Lines        6736     6750      +14     
==========================================
  Hits         3177     3177              
- Misses       3111     3125      +14     
  Partials      448      448              
Impacted Files Coverage Δ
plugins/source/testing.go 12.90% <0.00%> (-2.29%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions github-actions bot added feat and removed feat labels Feb 1, 2023
Copy link
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Nice, maybe you can share a PR that uses this?

@bbernays
Copy link
Contributor Author

bbernays commented Feb 1, 2023

Nice, maybe you can share a PR that uses this?

I am working on one now in the AWS plugin to check for the Tags issue we just solved... cloudquery/cloudquery#7621

@bbernays
Copy link
Contributor Author

bbernays commented Feb 5, 2023

@erezrokah The initial pr for utilizing this functionality is up... It already found 5 issues...

Copy link
Member

@hermanschaaf hermanschaaf left a comment

Choose a reason for hiding this comment

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

I'd probably define the Validator function signature as a type, but it's a minor nit 👍

Copy link
Contributor

@yevgenypats yevgenypats 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. Few thoughts on the validator interface

func(t *testing.T, table *schema.Table, resources *schema.Resource)

This way the SDK can do the same loop instead of the validator doing the loop again. Unless there is a need for the validator to have access to all tables but I feel it has those when it is calling the test anyway so feel a bit redundant.

Nit: per Herman comment type for the func would be nice.

@bbernays
Copy link
Contributor Author

bbernays commented Feb 6, 2023

Looks good. Few thoughts on the validator interface

func(t *testing.T, table *schema.Table, resources *schema.Resource)

This way the SDK can do the same loop instead of the validator doing the loop again. Unless there is a need for the validator to have access to all tables but I feel it has those when it is calling the test anyway so feel a bit redundant.

I wanted to give the validator access to all of the tables so that it could validate/ensure consistency across multiple tables... I thought that the hit for performance is negligible...

Nit: per Herman comment type for the func would be nice.

will look into this

@yevgenypats
Copy link
Contributor

Looks good. Few thoughts on the validator interface
func(t *testing.T, table *schema.Table, resources *schema.Resource)
This way the SDK can do the same loop instead of the validator doing the loop again. Unless there is a need for the validator to have access to all tables but I feel it has those when it is calling the test anyway so feel a bit redundant.

I wanted to give the validator access to all of the tables so that it could validate/ensure consistency across multiple tables... I thought that the hit for performance is negligible...

Nit: per Herman comment type for the func would be nice.

will look into this

But the validator already have access to all the tables because

Looks good. Few thoughts on the validator interface
func(t *testing.T, table *schema.Table, resources *schema.Resource)
This way the SDK can do the same loop instead of the validator doing the loop again. Unless there is a need for the validator to have access to all tables but I feel it has those when it is calling the test anyway so feel a bit redundant.

I wanted to give the validator access to all of the tables so that it could validate/ensure consistency across multiple tables... I thought that the hit for performance is negligible...

Nit: per Herman comment type for the func would be nice.

will look into this

But the validator already has in some sense access to all the tables. Anyway, if we want to pass the whole context to the validator, I think it makes sense but then let's just use the Plugin as a whole?

@bbernays bbernays requested a review from yevgenypats February 7, 2023 14:09
@kodiakhq kodiakhq bot merged commit 6b7b5de into cloudquery:main Feb 7, 2023
@bbernays bbernays deleted the custom-validators branch February 7, 2023 15:10
kodiakhq bot pushed a commit that referenced this pull request Feb 8, 2023
🤖 I have created a release *beep* *boop*
---


## [1.35.0](v1.34.0...v1.35.0) (2023-02-08)


### Features

* Enable Custom Validators ([#654](#654)) ([6b7b5de](6b7b5de))


### Bug Fixes

* **deps:** Update module golang.org/x/term to v0.5.0 ([#648](#648)) ([3a02bed](3a02bed))
* Handle null bytes in text fields ([8597f08](8597f08))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.

7 participants