Skip to content

Conversation

@ashh1215
Copy link

@ashh1215 ashh1215 commented Oct 6, 2025

Description

Added context parameter to all database query methods in driver files.

Fixes #319

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • go build passes for all modified drivers
  • go vet passes with no errors
  • Integration tests (attempted locally but encountered Docker connectivity issues - will rely on CI)

Documentation

  • Documentation Link: [link to README, olake.io/docs, or olake-docs]
  • N/A (bug fix, refactor, or test changes only)

@CLAassistant
Copy link

CLAassistant commented Oct 6, 2025

CLA assistant check
All committers have signed the CLA.

@vaibhav-datazip vaibhav-datazip added the hacktoberfest Issues open for Hacktoberfest contributors label Oct 7, 2025
@vaibhav-datazip
Copy link
Collaborator

vaibhav-datazip commented Oct 8, 2025

There is still a function in jdbc.go
inside the OracleIncrementalValueFormatter function , currently we are using

err = opts.Client.QueryRowContext(context.Background(), query).Scan(&datatype)

instead pass a ctx to the function and use that here.

Also you can run the sync for the drivers which you have modified, as test if all the functionalities are working fine.

@vaibhav-datazip
Copy link
Collaborator

LGTM

@vaibhav-datazip
Copy link
Collaborator

Hi @ashh1215 , can you please resolve these conflicts in your PR

Copy link
Collaborator

@vishalm0509 vishalm0509 left a comment

Choose a reason for hiding this comment

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

Hey @ashh1215
I saw that you did a force push recently. Looks like the merge conflicts were resolved, but not fully or correctly, few files still have issues after the merge.

Just a quick reminder from our contributing rules:

  • Please make sure all merge conflicts are properly resolved before pushing.
  • Try to avoid force pushes unless it’s really needed, since it can make the review history harder to follow.

Before pushing any changes, please do a quick round of local testing to confirm everything builds and works as expected.

Once that’s done, let me know and I’ll do another round of testing on my side. Thanks for your effort! 🙌

Copy link
Collaborator

@vishalm0509 vishalm0509 left a comment

Choose a reason for hiding this comment

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

please go through you code and remove the code which is not required in your branch, i can still see old code which is not present in current staging , reflecting that merge conflicts were not resolved correctly.

one way to help you out will be, you can check the context related changes and keep them and remove other changes.

@vaibhav-datazip vaibhav-datazip added the hacktoberfest-accepted This PR is a valid submission for hacktoberfest label Oct 16, 2025
@ashh1215 ashh1215 requested a review from vishalm0509 October 16, 2025 23:30
@shubham19may shubham19may changed the title improvement: add context while querying (sql/sqlx) (#319) fix: add context while querying (sql/sqlx) (#319) Oct 18, 2025
Copy link
Collaborator

@vishalm0509 vishalm0509 left a comment

Choose a reason for hiding this comment

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

LGTM

@hash-data hash-data merged commit 3831d53 into datazip-inc:staging Oct 20, 2025
10 of 12 checks passed
@ashh1215 ashh1215 deleted the AddContext branch October 26, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest Issues open for Hacktoberfest contributors hacktoberfest-accepted This PR is a valid submission for hacktoberfest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants