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

bugfix/mapping source connectors in destination cli commands #1788

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

rbiseck3
Copy link
Contributor

Description

Due to the dynamic nature of how the source connector is called when a destination command is invoked, the configs need to be mapped and the fsspec config needs to be dynamically added based on the type of runner being used. This code was added to all currently supported destination commands.

@ryannikolaidis
Copy link
Contributor

how did we not catch this in a test?
let's definitely add something that would have caught this.

@rbiseck3 rbiseck3 force-pushed the roman/bugfix-fsspec-source-dest-config branch 4 times, most recently from 4017576 to 1282113 Compare October 19, 2023 23:11
@rbiseck3
Copy link
Contributor Author

@ryannikolaidis, these will all be covered in the destination tests added in this PR: #1777

@ryannikolaidis
Copy link
Contributor

ryannikolaidis commented Oct 20, 2023

@ryannikolaidis, these will all be covered in the destination tests added in this PR: #1777

in the meantime, are there test instructions I can use to confirm it was broken before and fixed now?

@ahmetmeleq
Copy link
Contributor

Can we confirm we got these? If yes, LGTM

@rbiseck3
Copy link
Contributor Author

Test instructions: Run any fsspec source connector with any destination connector (other than s3). It'll fail to map the fsspec config.

Copy link
Contributor

@ahmetmeleq ahmetmeleq left a comment

Choose a reason for hiding this comment

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

LGTM!

@rbiseck3 rbiseck3 force-pushed the roman/bugfix-fsspec-source-dest-config branch from 1282113 to 51119e6 Compare October 20, 2023 16:44
Copy link
Contributor

@ryannikolaidis ryannikolaidis left a comment

Choose a reason for hiding this comment

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

looks good. please update the pr title as a conventional commit (since merge queue will leverage this)

@rbiseck3 rbiseck3 added this pull request to the merge queue Oct 20, 2023
Merged via the queue into main with commit 15b6969 Oct 20, 2023
39 checks passed
@rbiseck3 rbiseck3 deleted the roman/bugfix-fsspec-source-dest-config branch October 20, 2023 17:51
rbiseck3 added a commit that referenced this pull request Oct 20, 2023
Due to the dynamic nature of how the source connector is called when a
destination command is invoked, the configs need to be mapped and the
fsspec config needs to be dynamically added based on the type of runner
being used. This code was added to all currently supported destination
commands.
rbiseck3 added a commit that referenced this pull request Oct 20, 2023
Due to the dynamic nature of how the source connector is called when a
destination command is invoked, the configs need to be mapped and the
fsspec config needs to be dynamically added based on the type of runner
being used. This code was added to all currently supported destination
commands.
rbiseck3 added a commit that referenced this pull request Oct 20, 2023
Due to the dynamic nature of how the source connector is called when a
destination command is invoked, the configs need to be mapped and the
fsspec config needs to be dynamically added based on the type of runner
being used. This code was added to all currently supported destination
commands.
rbiseck3 added a commit that referenced this pull request Oct 21, 2023
Due to the dynamic nature of how the source connector is called when a
destination command is invoked, the configs need to be mapped and the
fsspec config needs to be dynamically added based on the type of runner
being used. This code was added to all currently supported destination
commands.
rbiseck3 added a commit that referenced this pull request Oct 23, 2023
Due to the dynamic nature of how the source connector is called when a
destination command is invoked, the configs need to be mapped and the
fsspec config needs to be dynamically added based on the type of runner
being used. This code was added to all currently supported destination
commands.
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