[WIP] feat(cli): Make consistent use of DataHubGraphClientConfig #10466
Conversation
github-actions
bot
added
the
ingestion
pedro93
requested review from
hsheth2,
asikowitz and
anshbansal
and removed request for
hsheth2 and
asikowitz
| @@ -541,6 +546,9 @@ def _relationships_endpoint(self): | |||
| def _aspect_count_endpoint(self): | |||
| return f"{self.config.server}/aspects?action=getCount" | |||
|
|
|||
| def _session(self) -> Session: | |||
| return super()._session | |||
There was a problem hiding this comment.
this doesn't look right
I don't think it's necessary, unless you want _session to be a method instead of an attr
There was a problem hiding this comment.
I was not able in the DataHubGraph object to pull attributes from the super class, so I built this function to surface that. At least the auto-import in Intellij wasn't finding it.
What would be the right way to surface this super's attribute?
There was a problem hiding this comment.
you should be able to do graph._session without any changes
if you really want a method, add a @property decorator
| return get_url_and_token()[1] | ||
|
|
||
|
|
||
| def get_session_and_host(): |
There was a problem hiding this comment.
to reduce the amount of code changes, you could make this call get_default_graph() and then return the tuple
basically keep it as a compatibility shim
There was a problem hiding this comment.
Doing that causes a cyclical import dependency. It was my first implementation attempt.
There was a problem hiding this comment.
you can put the import inside the get_session_and_host method to avoid that issue
Deprecates
get_url_and_token()in favor of a more complete option:load_graph_config()that returns a fullDatahubClientConfig.This change was then propagated across previous usages of
get_url_and_tokenso that connections to DataHub server from the client respect the full breadth of configuration specified byDatahubClientConfig.I.e: You can now specify
disable_ssl_verification: truein your~/.datahubenvfile so that all cli functions to the server work when ssl certification is disabled.Checklist