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

Fixes the display name issues for tables, and tags count #69

Merged
merged 4 commits into from Aug 14, 2019

Conversation

verdan
Copy link
Member

@verdan verdan commented Aug 6, 2019

Summary of Changes

WIP: Made some changes in atlasclient, waiting for them to be fixed and released before this change.
This PR involves three changes:

  • Fixes the table display name for Atlas Proxy.
  • Add support for tags count.
  • Fixes the setup.py requirements package synching issue.

Tests

Based on the changes above, I needed to modify the atlas proxy test cases.

Documentation

What documentation did you add or modify and why? Add any relevant links then remove this line

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
  • PR passes make test
  • I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message"

@codecov-io
Copy link

codecov-io commented Aug 8, 2019

Codecov Report

Merging #69 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #69   +/-   ##
=======================================
  Coverage   74.11%   74.11%           
=======================================
  Files          19       19           
  Lines         989      989           
  Branches       84       84           
=======================================
  Hits          733      733           
  Misses        234      234           
  Partials       22       22
Impacted Files Coverage Δ
metadata_service/config.py 100% <ø> (ø) ⬆️
metadata_service/proxy/atlas_proxy.py 88.27% <100%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 493cb79...8973de3. Read the comment docs.

@verdan verdan changed the title [WIP] Fixes the display name issues for tables, and tags count Fixes the display name issues for tables, and tags count Aug 8, 2019
@verdan
Copy link
Member Author

verdan commented Aug 8, 2019

@feng-tao @jinhyukchang ready for review

Update: Added some helper functions in atlasclient, that would make things easier. waiting for the next release of atlasclient.

@verdan verdan changed the title Fixes the display name issues for tables, and tags count WIP: Fixes the display name issues for tables, and tags count Aug 9, 2019
@feng-tao
Copy link
Member

@verdan CI fails

@verdan
Copy link
Member Author

verdan commented Aug 10, 2019

@feng-tao yes, because the new atlasclient is not yet released. Will be fixed once that's there. Will ping you once done.

@verdan verdan changed the title WIP: Fixes the display name issues for tables, and tags count Fixes the display name issues for tables, and tags count Aug 13, 2019
@verdan
Copy link
Member Author

verdan commented Aug 13, 2019

@feng-tao ready for review. We need to fork the atlasclient as the maintainer was not active enough, and we want to speed up the development on Amundsen-Atlas now.

Copy link
Member

@feng-tao feng-tao left a comment

Choose a reason for hiding this comment

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

just a few questions. LGTM

@@ -18,23 +19,18 @@
LOGGER = logging.getLogger(__name__)


# noinspection PyMethodMayBeStatic
Copy link
Member

Choose a reason for hiding this comment

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

just curious: what is this line about?

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes pycharm not to give warning about the method being static.

@@ -59,4 +59,5 @@ neo4j-driver==1.6.0
neotime==1.0.0
pytz==2018.4
statsd==3.2.1
atlasclient==0.1.7
pyatlasclient==1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

so this will be the new deps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. We'll be working on this one. (for all microservices)


requirements_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'requirements.txt')
with open(requirements_path) as requirements_file:
requirements = requirements_file.readlines()
Copy link
Member

Choose a reason for hiding this comment

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

actually there is some concern over pin the deps in amundsen-io/amundsen#80

@feng-tao
Copy link
Member

thanks @verdan !

@feng-tao feng-tao merged commit c79d191 into amundsen-io:master Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants