feat: Implement generic pipelining utility #6683
Conversation
evanpurkhiser
changed the base branch from
master
to
feat-add-generic-toobigforsession-object-store-utility
Generated by 🚫 danger |
|
@lauryndbrown, @alex-hofsteede: Mind if I loop you guys in to take a look at this. Trying to get a little bit of knowledge share going around pieces related to authentication. |
evanpurkhiser
requested review from
alex-hofsteede and
lauryndbrown
and removed request for
mattrobenolt
|
@evanpurkhiser ack, having a look now |
src/sentry/utils/pipeline.py
Outdated
| class ProviderPipeline(object): | ||
| """ | ||
| ProviderPipeline provides a mechanism to guide the user through a request | ||
| 'pipeline', where each view may be copleted by calling the ``next_step`` |
src/sentry/utils/pipeline.py
Outdated
| return nested_pipeline.current_step() | ||
|
|
||
|
|
||
| class ProviderPipeline(object): |
There was a problem hiding this comment.
ProviderPipeline and PipelineableProvider class names are a little confusing, although I saw you chat about this in slack yesterday so I assume you've already thought this one through
There was a problem hiding this comment.
I think what i'll do is just rename ProviderPipeline to Pipeline.
src/sentry/utils/pipeline.py
Outdated
| pipeline views that the ProviderPipeline will traverse through. | ||
| """ | ||
|
|
||
| def get_pipeline(self): |
There was a problem hiding this comment.
is this more aptly named get_views?
There was a problem hiding this comment.
Going to go with get_pipleine_views. bf734e7. I think it's a little more clear.
tests/sentry/utils/test_pipeline.py
Outdated
|
|
||
|
|
||
| class DummyProviderManager(object): | ||
| def get(self, provider): |
tests/sentry/utils/test_pipeline.py
Outdated
|
|
||
| class DummyProviderManager(object): | ||
| def get(self, provider): | ||
| return DummyProvider() |
There was a problem hiding this comment.
Just for completeness of testing, can this return a provider from a keyed list? eg.
return {'dummy': DummyProvider()}.get(provider_key)
There was a problem hiding this comment.
I don't really have the context for how this will be used in practice so forgive me if this is off the mark, but I'm not sure why we need a ProviderManager to fetch a PipelineableProvider to fetch a list of PipelineViews. Could we dispense with 2 of these levels of abstraction and just initialize our ProviderPipeline with the list of views directly? Or are there more complex use cases in the works that need multiple types of providers and provider managers?
There was a problem hiding this comment.
Just for completeness of testing, can this return a provider from a keyed list
love it fe31613
but I'm not sure why we need a
ProviderManagerto fetch aPipelineableProviderto fetch a list ofPipelineViews. Could we dispense with 2 of these levels of abstraction and just initialize ourProviderPipelinewith the list of views directly? Or are there more complex use cases in the works that need multiple types of providers and provider managers?
Yes, the primary use for the pipeline utility class is to handle OAuth (and SAML) auth flows, as well as any additional setup screens (for example, with github SSO setup you have to select the GH organization you want to do SSO with).
So in that case, the manager is used to lookup what provider pipeline should be used (for example, if logging in via SSO it would use the a provider keyed on 'github', 'google', etc). Then some of the views themselves are sometimes shared (for example, the OAuth pipeline provider shares a view for callback handling)
There are a few different pipelines (Auth: For SSO login authentication and SSO auth setup, Identity: For handling the actual identity lookup through identity providers like google, and Integration: for setting up an integration (which similar to auth, composes identity pipelines).
This should make more sense with the upcoming identity and integration stuff I'm finishing up.
tests/sentry/utils/test_pipeline.py
Outdated
|
|
||
| # Piepline has two steps, ensure both steps compete. | ||
| # Usually the dispatch itself would be the one calling the current_step | ||
| # and next_step methods after it determins if it can move forward a |
tests/sentry/utils/test_pipeline.py
Outdated
| provider_manager = DummyProviderManager() | ||
|
|
||
| def finish_pipeline(self): | ||
| self.finished = 1 |
| pipeline.next_step() | ||
| assert pipeline.dispatch_count == 2 | ||
|
|
||
| pipeline.next_step() |
There was a problem hiding this comment.
It seems a little strange to have to call current_step once and then next_step subsequently for this pipeline. It might be nicer if the logic in those functions was set up so that you could just do:
while pipeline.next_step():
...
There was a problem hiding this comment.
ah I see your comment about dispatch
There was a problem hiding this comment.
This test is a little bit convoluted from usually usage unfortunately. Usually the pipeline view itself would be calling next_step and then the initiating view that gets / creates the pipeline would call current_step.
evanpurkhiser
changed the base branch from
feat-add-generic-toobigforsession-object-store-utility
to
master
This will serve as a base class for pipelines used in the identity association process, integration setup, and in the future replace the auth provider setup / login pipeline.
Also adds a method specifically for this
evanpurkhiser
force-pushed
the
feat-implement-generic-pipelining-utility
branch
from
21a60e6 to
06499a1
Compare
This is a generic implementation of the pipeline helper (renaming to just pipeline) used in the auth and integrations modules. This extracts most of the pipelineing and state managment into a single base class that can be subclassed for specific pipeline types.
Future PRs will 1) refactor the auth and integration module to use this pipeline utility, and 2) make use of the pipeline utility for a new identity pipeline.