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/enable-handle-fs-connector #456

Merged
merged 23 commits into from
Jul 15, 2024
Merged

Conversation

kirtimanmishrazipstack
Copy link
Contributor

@kirtimanmishrazipstack kirtimanmishrazipstack commented Jul 5, 2024

What

  • Azure FS, GCS have been enabled
  • Made necessary changes to comprehend all unstract fs connector
  • Remove path in minio, not used
  • Remove overwrite parameter from destination connector

Why

  • Remove path in minio, because path is not used. In test connection, we are only using bucket.
  • Remove overwrite parameter from destination connector. This is done because all fsspec connector is not using overwrite parms. Connectors like Dropbox, minio, GCS, Azure FS will always overwrite the file if present. Where as Gdrive always creates a new file if already present. So this param is not needed in destination connector

How

  • Azure FS, GCS have been enabled from FE
  • Made necessary changes in backend/workflow_manager/endpoint/source.py and backend/workflow_manager/endpoint/destination.py to comprehend all unstract fs connector
  • Remove path in minio from unstract/connectors/filesystems/minio/minio.py
  • Remove overwrite param backend/workflow_manager/endpoint/static/dest/file.json to remove overwrite from destination connector

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • This PR only affects the FS connector. All FS connector have tested on local

Database Migrations

  • N/A

Env Config

  • N/A

Relevant Docs

Related Issues or PRs

Dependencies Versions

Notes on Testing

  • Tested all 5 FS connectors as a source and destination on workflow

Screenshots

Checklist

I have read and understood the Contribution Guidelines.

@kirtimanmishrazipstack kirtimanmishrazipstack requested review from a team, ritwik-g, harini-venkataraman, chandrasekharan-zipstack and muhammad-ali-e and removed request for a team July 5, 2024 10:35
@kirtimanmishrazipstack kirtimanmishrazipstack marked this pull request as ready for review July 5, 2024 10:36
Copy link
Contributor

@chandrasekharan-zipstack chandrasekharan-zipstack left a comment

Choose a reason for hiding this comment

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

Changes look good for the most part, however do confirm if this requires any migration to avoid regressions on removing the parameters. I believe we persist these details in the DB

backend/workflow_manager/endpoint/base_connector.py Outdated Show resolved Hide resolved
backend/workflow_manager/endpoint/fs_connector_helper.py Outdated Show resolved Hide resolved
@kirtimanmishrazipstack
Copy link
Contributor Author

kirtimanmishrazipstack commented Jul 8, 2024

Changes look good for the most part, however do confirm if this requires any migration to avoid regressions on removing the parameters. I believe we persist these details in the DB

@chandrasekharan-zipstack I believe we are storing as json in workflow_endpoints table. When I remove the params, its will only change the json. So no migration changes as per my understanding.
@muhammad-ali-e let me know your thoughts

Screenshot from 2024-07-08 10-10-06

Copy link
Contributor

@muhammad-ali-e muhammad-ali-e left a comment

Choose a reason for hiding this comment

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

@kirtimanmishrazipstack Overall LGTM. But Please try to move the functions or methods that related to specific connectors in to their own classes or their base classes

backend/workflow_manager/endpoint/fs_connector_helper.py Outdated Show resolved Hide resolved
backend/workflow_manager/endpoint/destination.py Outdated Show resolved Hide resolved
@muhammad-ali-e
Copy link
Contributor

@chandrasekharan-zipstack I believe we are storing as json in workflow_endpoints table. When I remove the params, its will only change the json. So no migration changes as per my understanding.
@muhammad-ali-e let me know your thoughts

Yes I beleive so.It will not affect anything if it not used anywhere.

Copy link
Contributor

@ritwik-g ritwik-g left a comment

Choose a reason for hiding this comment

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

@kirtimanmishrazipstack I have only one comment. Other than that it looks fine.

But please make sure to address comments from @muhammad-ali-e before merging

Copy link
Contributor

@muhammad-ali-e muhammad-ali-e left a comment

Choose a reason for hiding this comment

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

LGTM, Please ensure thorough testing with both new and existing connectors.

@kirtimanmishrazipstack kirtimanmishrazipstack requested a review from a team July 11, 2024 05:08
Copy link

filepath function $$\textcolor{#23d18b}{\tt{passed}}$$ SUBTOTAL
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_logs}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup\_skip}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_client\_init}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_exists}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config\_without\_mount}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_run\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{9}}$$ $$\textcolor{#23d18b}{\tt{9}}$$

Copy link

sonarcloud bot commented Jul 15, 2024

@hari-kuriakose hari-kuriakose merged commit 4a677f0 into main Jul 15, 2024
5 checks passed
@hari-kuriakose hari-kuriakose deleted the fix/enable-handle-fs-connector branch July 15, 2024 10:59
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.

None yet

6 participants