-
Notifications
You must be signed in to change notification settings - Fork 4k
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(destination-pinecone): Replaced using AirbyteLogger
with an AirbyteLogMessage
instead
#40753
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
@aaronsteers I assume integration tests will run on CI, right? |
I believe so... There's a lot going on here, but I assume integration tests would run here: https://github.com/airbytehq/airbyte/actions/runs/9813423151/job/27099424463 UPDATE: No. And I have no idea why. My first guess was missing test config, but no, it looks like that is present: airbyte/airbyte-integrations/connectors/destination-pinecone/metadata.yaml Lines 44 to 60 in 9ee71ec
UPDATE (2): See below. Build fails from a Pydantic error, then other steps are skipped. |
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.
@natikgadzhi - Looks like Pydantic 2.0 (aka CDK 2.0) breaks this.
RuntimeError: no validator found for <class 'destination_pinecone.config.PineconeIndexingModel'>, see `arbitrary_types_allowed` in Config
☝️ from the CI report.
Yep, pinning to <2. |
/format-fix
|
@aaronsteers ready. Fixing formatting and version, but tests should pass now. |
/format-fix
|
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.
Looks great - thanks, @natikgadzhi! 🙏
@natikgadzhi - I see another release landed last night with the same version number for this connector, which is blocking this one now. I will see if I can resolve the conflict real quick. Update: Done! |
…mp version to 0.1.9, re-run poetry lock)
What
When handling an error, destination Pinecone attemted to use AirbyteLogger, which we removed before CDK 1.0, so that blew up.
This fix uses
AirbyteLogMessage
instead. I've also updated CDK to 2.0 while I'm here.User Impact
When connector errors out, now it should capture the error instead of crashing.
Can this PR be safely reverted and rolled back?