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

Fix: Add schema in Postgres Cache to swapping method #149

Merged

Conversation

tinomerl
Copy link
Contributor

Fixes #148

I dug a bit around and found out, that the Problem was, that the schema name was missing in the final swaping. I just added the method used at other places to add the schema.

I separated the statements in several lines so that i won't go over the defined Line Limit from 100.

Tests were all successful except for the integration Tests of some caches, since i don't have access to them.
image

Also tested the fix manuallay with the pokeapi source and the linkedinpages source mentioned in #148. Everything worked and data was in the final tables.

@aaronsteers
Copy link
Contributor

aaronsteers commented Mar 28, 2024

/test-pr

Full test output (internal link): https://github.com/airbytehq/PyAirbyte/actions/runs/8470339797/job/23207828832

@aaronsteers
Copy link
Contributor

aaronsteers commented Mar 28, 2024

@tinomerl - This is great. Thank you!

I found a small typo and just applied the fix (adding back the space between "RENAME" and "TO").

Re-running our test suite here: https://github.com/airbytehq/PyAirbyte/actions/runs/8470606404

I'm not sure if you'll be able to see summary status or if that link will work at all. But I'll watch it and report back. Once tests pass, we should be clear to merge.

I'm also using this PR as a test for ability to run CI on forks. I merged in a fix from main that should allow them to be run correctly. I can see now that the "Fast" and "No-Creds" versions of the Pytest CI workflows are working correctly and getting the green checkmark. ✅ 🙌

@tinomerl
Copy link
Contributor Author

@aaronsteers - thanks! I'm happy to help.

Oh darn. I saw the typo. Should have caught that one myself by looking at the very first runs. 🙈

The link for the rerun works for me and I can see the results.

Great. That means it's easier and faster for you guys to check PRs from forks right?

@aaronsteers
Copy link
Contributor

@tinomerl - Just for my info - can you confirm if you can see results only, or also the detailed logs?

@aaronsteers
Copy link
Contributor

Great. That means it's easier and faster for you guys to check PRs from forks right?

Exactly that. 😄

@aaronsteers
Copy link
Contributor

aaronsteers commented Mar 28, 2024

Will merge shortly.

Tests are green here in the PR (auto-skipping tests that require creds), and tests are also green on the manual workflow here (which uses our creds to do the full suite of tests).

image

@tinomerl
Copy link
Contributor Author

tinomerl commented Mar 28, 2024

@tinomerl - Just for my info - can you confirm if you can see results only, or also the detailed logs?

@aaronsteers I can also see the logs of every step when clicking it. But only after it's finished, if this makes any difference.

@aaronsteers aaronsteers merged commit 3e5cc4b into airbytehq:main Mar 28, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: PostgreSQL Cache can't rename intermediate Table after replication
2 participants