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

[Core] Config not required for pipeline client #6972

Closed
wants to merge 4 commits into from

Conversation

annatisch
Copy link
Member

No description provided.

@adxsdk6
Copy link

adxsdk6 commented Aug 28, 2019

Can one of the admins verify this patch?

@bryevdv
Copy link
Contributor

bryevdv commented Aug 28, 2019

This seems fine though I am curious what are all the use cases? When would a Pipeline vs a config vs a list of policies be used?

@annatisch
Copy link
Member Author

Thanks @bryevdv!
Hmm well this is kind of cleaning up the code that's there - we allow users to pass in policies or a pipeline, and therefore there would be no need to pass in a config.
I think for storage we build the pipeline ourselves and pass it in. In the case of cosmos we don't use any kind of configuration object, so pass in a list of policies. My understanding is that the config object itself is being phased out - it's already been removed from public-facing APIs.

Copy link
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

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

Please take a look

@@ -59,7 +59,7 @@ class PipelineClient(PipelineClientBase):
Builds a Pipeline client.

:param str base_url: URL for the request.
:param config: Service configuration. This is a required parameter.
:param config: Service configuration. This is a required unless pipeline or policies are given separately.
Copy link
Member

Choose a reason for hiding this comment

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

Missing "parameter"?

@@ -80,10 +80,8 @@ class PipelineClient(PipelineClientBase):
:caption: Builds the pipeline client.
"""

def __init__(self, base_url, config, **kwargs):
def __init__(self, base_url, config=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Should we put config into kwargs?

DistributedTracingPolicy(),
config.logging_policy,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add proxy policy as well.

@@ -106,6 +104,8 @@ def _build_pipeline(self, config, **kwargs): # pylint: disable=no-self-use
policies = kwargs.get('policies')

if policies is None: # [] is a valid policy list
if config is None:
raise ValueError("No config or policies supplied.")
Copy link
Member

Choose a reason for hiding this comment

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

Why not if config is None, give a default one?

@annatisch
Copy link
Member Author

This PR isn't needed for Preview3 - so going to shelve for now.

@annatisch
Copy link
Member Author

Picked up in PR #7734

@annatisch annatisch closed this Oct 9, 2019
@annatisch annatisch deleted the annatisch-patch-2 branch October 9, 2019 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants