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

Update dependencies, fix conflicts with other packages #764

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

kenshaw
Copy link
Contributor

@kenshaw kenshaw commented Mar 30, 2023

Description

Changes arrow to v11, updates all dependencies, and removes replace statements within go.mod, as these cause conflicts/issues when importing this package.

Checklist

  • Code compiles correctly
  • Run make fmt to fix inconsistent formats
  • Run make lint to get lint errors and fix all of them
  • [N/A] Created tests which fail without the change (if possible)
  • All tests passing
  • [N/A] Extended the README / documentation, if necessary

@kenshaw kenshaw requested a review from a team as a code owner March 30, 2023 01:57
@github-actions
Copy link

github-actions bot commented Mar 30, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@kenshaw
Copy link
Contributor Author

kenshaw commented Mar 30, 2023

I have read the CLA Document and I hereby sign the CLA

@kenshaw
Copy link
Contributor Author

kenshaw commented Mar 30, 2023

Submitting this PR as there are conflicts with Snowflake's dependency on the older arrow version with other drivers (which also make use of arrow) in usql.

I would humbly request/suggest that this package drop the vendor directory, as it would be much preferable if dependencies (like database drivers) left this up to the project incorporating the package.

@zeroshade
Copy link
Contributor

The failure here:
image
appears to be an issue on the CI side, rather than an issue with the actual change/diffs here. Could this please get looked at and expedited?

Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

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

LGTM

@sfc-gh-igarish
Copy link
Collaborator

Please update the branch.

@zeroshade
Copy link
Contributor

@kenshaw can you please update / rebase this branch? This should get merged before my #769 does

Changes arrow to v11, updates all dependencies, and removes replace
statements within go.mod, as these cause conflicts/issues when importing
this package.
@kenshaw
Copy link
Contributor Author

kenshaw commented Apr 19, 2023

@zeroshade done.

@zeroshade
Copy link
Contributor

@sfc-gh-igarish branch is updated! please merge at your earliest convenience!

@sfc-gh-igarish
Copy link
Collaborator

Still waiting for test to finish and pass.

@zeroshade
Copy link
Contributor

The tests that failed in the CI don't appear to be anything related to this change as far as I can tell (unless there are other tests that are running but that aren't publicly visible on here?)

image

It looks like the issue is something with the config and secret key?

@kenshaw
Copy link
Contributor Author

kenshaw commented Apr 19, 2023

@zeroshade I would assume that's because this is PR, and those secrets are available for PRs.

@sfc-gh-igarish
Copy link
Collaborator

sfc-gh-igarish commented Apr 19, 2023

ok. I get your PR and created a new PR on top of it and link with your PR. Let's see how tests goes now here.

@sfc-gh-igarish sfc-gh-igarish merged commit 34eeb45 into snowflakedb:master Apr 19, 2023
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.

3 participants