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

Revert "feat: Integrating thirdy party Slack API " #971

Merged
merged 1 commit into from
Aug 27, 2023

Conversation

jiashenC
Copy link
Member

No description provided.

@jiashenC
Copy link
Member Author

jiashenC commented Aug 27, 2023

Is the previous PR #967 in a ready state? @kaushikravichandran

@kaushikravichandran
Copy link
Collaborator

kaushikravichandran commented Aug 27, 2023

Hi @jiashenC . The previous PR does not affect any component. I merged it because Aryan has to build on those changes. He had taken a look at the PR.

@jiashenC jiashenC requested a review from xzdandy August 27, 2023 14:31
@jiashenC
Copy link
Member Author

Hi @jiashenC . The previous PR does not affect any component. I merged it because Aryan has to build on those changes. He had taken a look at the PR.

I think it would be better that you merge this into Aryan's branch first and then merge everything together into staging. What do you think @xzdandy?

In addition, there are several things that can be improved for this PR

  1. We'd like to add unit tests for each third-party handler.
  2. Please remove slack_sdk from setup.py. Instead, it can be installed based on the handler by adding a requirements.txt. Here is the reference https://github.com/georgia-tech-db/evadb/tree/staging/evadb/third_party/databases/postgres.

@xzdandy
Copy link
Collaborator

xzdandy commented Aug 27, 2023

Hi @jiashenC . The previous PR does not affect any component. I merged it because Aryan has to build on those changes. He had taken a look at the PR.

I think it would be better that you merge this into Aryan's branch first and then merge everything together into staging. What do you think @xzdandy?

In addition, there are several things that can be improved for this PR

  1. We'd like to add unit tests for each third-party handler.
  2. Please remove slack_sdk from setup.py. Instead, it can be installed based on the handler by adding a requirements.txt. Here is the reference https://github.com/georgia-tech-db/evadb/tree/staging/evadb/third_party/databases/postgres.

Yes I agree. Let's only merge the fully ready PR to staging.

@jiashenC jiashenC merged commit 405b773 into staging Aug 27, 2023
@jiashenC jiashenC deleted the revert-967-kravicha3/slack_api_interface branch August 27, 2023 16:55
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