Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for get all tables wildcard #51

Merged
merged 6 commits into from
Feb 7, 2025

Conversation

nickchomey
Copy link
Contributor

@nickchomey nickchomey commented Sep 28, 2024

Description

This adds support for specifying "*" in the config.Tables field, which will then allow for snapshotting and syncing from all tables in the specified database.

Fixes #29

Quick checks:

  • There is no other pull request for the same update/change.
  • I have NOT written unit tests. I don't know how...
  • I have made sure that the PR is of reasonable size and can be easily reviewed.

Copy link
Contributor

@hariso hariso left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution @nickchomey!

@nickchomey
Copy link
Contributor Author

Thanks! It still needs tests though

And I suspect this PR will be superseded/made irrelevant by what we've discussed in #54. I know that somewhere there we discussed just using all tables by default if the includes is blank - similar to how gitignore works.

Though I got inspiration for this wildcard from the Postgres connector, so perhaps it would be best to follow a similar pattern here?

Whatever the case, I'd prefer to solve #54 first and implement tests there, then see if this one is still needed.

Copy link
Collaborator

@alarbada alarbada left a comment

Choose a reason for hiding this comment

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

lgtm @nickchomey!

@maha-hajja
Copy link
Contributor

Hi @nickchomey! can you please update this PR and resolve the conflicts so we can go ahead and merge it, thanks!

@nickchomey
Copy link
Contributor Author

nickchomey commented Feb 6, 2025

Sure, glad to hear you're still interested in it! I'll try to do it in the next day or so. I haven't touched conduit in a few months, so I'm looking forward to exploring the performance improvements and new cli.

Will there be interest in the include/exclude regex patterns in #54?

@nickchomey
Copy link
Contributor Author

Ive merged the latest version of the connector to this PR. Let me know if you need anything else!

@raulb
Copy link
Contributor

raulb commented Feb 7, 2025

Thank you @nickchomey !! 🚀

@raulb
Copy link
Contributor

raulb commented Feb 7, 2025

@nickchomey looks like CI is not happy about a couple of things. Seems pretty minor. Let me know if you need assistance

  Error: source.go:175:1: File is not properly formatted (gofumpt)
  
  ^
  Error: source.go:174:24: Rows/Stmt/NamedStmt was not closed (sqlclosecheck)
  	rows, err := db.Queryx(query)

auto-merge was automatically disabled February 7, 2025 15:18

Head branch was pushed to by a user without write access

@nickchomey
Copy link
Contributor Author

Ive resolved the gofumpt (an extra blank line) and I believe ive resolved the paramaterized query. Please let me know if you need anything else.

@lovromazgon lovromazgon merged commit 0cf347a into conduitio-labs:main Feb 7, 2025
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: support wildcards in table field
6 participants