-
Notifications
You must be signed in to change notification settings - Fork 43
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
Use ProviderManager in webhook handler #3202
Conversation
Relates to #2845
@@ -864,9 +846,8 @@ func getPullRequestInfoFromPayload( | |||
}, nil | |||
} | |||
|
|||
func reconcilePrWithDb( | |||
func (s *Server) reconcilePrWithDb( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed some functions to methods so we do not need to pass around the db.Store
pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long-term, we want these to be in the provider implementation and not the server, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually, yes.
@@ -246,27 +243,6 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() { | |||
projectID := uuid.New() | |||
providerID := uuid.New() | |||
|
|||
// create a test oauth2 token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needed because the token querying is not triggered unless the provider is actually instantiated. The test case for the handler does not need the provider to be instantiated.
action string, | ||
) error { | ||
// NOTE(jaosorior): this webhook is very specific to github |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is still true, we just think it's obvious, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - but this particular codepath is so Github-specific that it's not really worth noting any more.
@@ -864,9 +846,8 @@ func getPullRequestInfoFromPayload( | |||
}, nil | |||
} | |||
|
|||
func reconcilePrWithDb( | |||
func (s *Server) reconcilePrWithDb( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long-term, we want these to be in the provider implementation and not the server, correct?
Relates to #2845
Switch to the use of the new ProviderManager interface in webhook handler to instantiate the Github provider.
Summary
Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.
Fixes #(related issue)
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: