-
Notifications
You must be signed in to change notification settings - Fork 145
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: Ignore Catalog Cache #8518
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for meltano ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thanks @andyoneal! I'll make some time to review :D |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8518 +/- ##
==========================================
+ Coverage 91.81% 92.02% +0.20%
==========================================
Files 245 246 +1
Lines 19296 19410 +114
Branches 2152 2157 +5
==========================================
+ Hits 17717 17862 +145
+ Misses 1306 1272 -34
- Partials 273 276 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andyoneal!
Can we add this to the documentation for the "run" command? https://docs.meltano.com/reference/command-line-interface#run
The new extra is probably fine, and it's a nice way for users to tell Meltano to always use a fresh catalog. |
Co-authored-by: Edgar Ramírez Mondragón <16805946+edgarrmondragon@users.noreply.github.com>
…ect-cli-select-service test: Add more tests for `SelectService` and the `select` command
@andyoneal I think files in https://github.com/meltano/meltano/tree/42405debc9369235c64c2f5a0ad8fbd9a1e61a35/integration/meltano-manifest/expected-manifests need to be updated so the integration tests can pass |
was wondering where those were coming from. will update. |
@@ -185,6 +185,9 @@ class SingerTap(SingerPlugin): # noqa: WPS214 | |||
value_processor="nest_object", | |||
), | |||
SettingDefinition(name="_select_filter", kind=SettingKind.ARRAY, value=[]), | |||
SettingDefinition( | |||
name="_no_catalog_cache", kind=SettingKind.BOOLEAN, value=False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit that came up while checking this out during Office Hours:
What do you think of making this _catalog_cache
instead and default it to True
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All else being equal, avoiding a double negative would be nice
closes #6919
no_catalog_cache
to config extras, so that a single plugin can be run every time without the catalog cache.--catalog-refresh
option tomeltano select
andmeltano el
Adding a new extra seems extreme, but I don't know how else to implement this where it could be used as part of a job for any tap. If there's a better way to do it, I'm happy to adjust this based on feedback.