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

[refactor] - Remove GMS GraphQL Service #3605

Merged

Conversation

arunvasudevan
Copy link
Collaborator

@arunvasudevan arunvasudevan commented Nov 21, 2021

Checklist

  • [X ] The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

Back in Jan'21 when GMS did not have a graphql endpoint this service was added, now that GMS itself hosts both Rest.li and GraphQL endpoints there i snot reason for this service to exist. So, cleaning it up.

Also, found a JQ dependency while installing datahub so added to the quickstart.

@github-actions
Copy link

github-actions bot commented Nov 21, 2021

Unit Test Results

     37 files  ±0       37 suites  ±0   28m 46s ⏱️ - 1m 21s
   591 tests ±0     539 ✔️ ±0  52 💤 ±0  0 ±0 
1 328 runs  ±0  1 254 ✔️ ±0  74 💤 ±0  0 ±0 

Results for commit 0a87112. ± Comparison against base commit e6b4343.

♻️ This comment has been updated with latest results.

@arunvasudevan
Copy link
Collaborator Author

Looks like flaky tests..need a re-run

@jjoyce0510
Copy link
Collaborator

Yayyyyy! Thank you Arun!

@@ -4,7 +4,7 @@

To deploy a new instance of DataHub, perform the following steps.

1. Install [docker](https://docs.docker.com/install/) and [docker-compose](https://docs.docker.com/compose/install/) (if
1. Install [docker](https://docs.docker.com/install/), [jq](https://stedolan.github.io/jq/download/) and [docker-compose](https://docs.docker.com/compose/install/) (if
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!!

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

Overall this LGTM. Thank you so much @arunvasudevan.

@jjoyce0510
Copy link
Collaborator

Seems that CI is failing - once that is resolved this should be good

@jjoyce0510
Copy link
Collaborator

It's not clear to me why it's failing, @arunvasudevan can you merge latest master into this branch and try once more? Seems unrelated to your changes: https://github.com/linkedin/datahub/runs/4279078587?check_suite_focus=true

@arunvasudevan
Copy link
Collaborator Author

Sure!

@arunvasudevan arunvasudevan force-pushed the remove_gms_graphql_service branch from cd5e291 to 0a87112 Compare November 22, 2021 21:07
@arunvasudevan
Copy link
Collaborator Author

@jjoyce0510 CI Looks good now.

@jjoyce0510
Copy link
Collaborator

LGTM. Thanks again!

@shirshanka shirshanka merged commit fde42e0 into datahub-project:master Nov 22, 2021
@arunvasudevan arunvasudevan deleted the remove_gms_graphql_service branch November 23, 2021 17:39
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