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

feat: Actual providers implementation #1011

Merged
merged 1 commit into from
Sep 26, 2023
Merged

feat: Actual providers implementation #1011

merged 1 commit into from
Sep 26, 2023

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Sep 25, 2023

This defines the provider implementations in such a way that we can instantiate them and check if a rule type is able to accomodate for those.

This is done through a new ProviderBuidler implementation which is in charge of instantiating the provider type that's needed at a given time.

Note that we still use the GitHub provider for HTTP calls since it has a lot of handy features (such as rate limiting handling) that we could leverage.

Note that on the way, this removes the GitHub graphql client implementation since it's unused. We may recover it from history if/when needed.

Related-to: #723
Closes: #543

@JAORMX JAORMX requested a review from jhrozek September 25, 2023 12:13
@JAORMX JAORMX changed the title Actual providers implementation WIP: Actual providers implementation Sep 25, 2023
@JAORMX JAORMX force-pushed the providers-p2 branch 4 times, most recently from c0c779b to f7173c0 Compare September 25, 2023 12:45
@JAORMX
Copy link
Contributor Author

JAORMX commented Sep 25, 2023

This works in a basic sense, but I still need to implement the HTTP provider and add configuration options support (which will be read from the DB)

@JAORMX JAORMX force-pushed the providers-p2 branch 4 times, most recently from 0478927 to c90a352 Compare September 25, 2023 14:30
@JAORMX
Copy link
Contributor Author

JAORMX commented Sep 25, 2023

Added basic configurations from the database already.

This is ready for feedback @evankanderson @jhrozek @rdimitrov

Base automatically changed from providers-init to main September 25, 2023 15:55
@JAORMX JAORMX force-pushed the providers-p2 branch 2 times, most recently from eff756e to 263f087 Compare September 25, 2023 16:04
@JAORMX JAORMX changed the title WIP: Actual providers implementation Actual providers implementation Sep 25, 2023
@JAORMX JAORMX force-pushed the providers-p2 branch 2 times, most recently from 3571999 to e60f070 Compare September 26, 2023 04:00
@JAORMX JAORMX changed the title Actual providers implementation feat: Actual providers implementation Sep 26, 2023
rdimitrov
rdimitrov previously approved these changes Sep 26, 2023
Copy link
Member

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

Can we expect that mediator will continue to fetch updates via a webhook? In that sense would it be useful if we update the current webhook handler to be more provider agnostic by adding to the provider interface methods for populating the initial event message (something similar to what we do when we build the entity wrapper?) but also have a method for processing a web hook event. Anyway, the PR is big enough so I'll approve to not block it 👍

func (h *REST) Do(ctx context.Context, req *http.Request) (*http.Response, error) {
req = req.WithContext(ctx)

return h.cli.Do(req)
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 close it too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Closing should happen at the caller's side (the ingester)

jhrozek
jhrozek previously approved these changes Sep 26, 2023
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

looks good, two questions inline

internal/providers/providers.go Outdated Show resolved Hide resolved
internal/providers/providers.go Outdated Show resolved Hide resolved
@JAORMX
Copy link
Contributor Author

JAORMX commented Sep 26, 2023

Can we expect that mediator will continue to fetch updates via a webhook? In that sense would it be useful if we update the current webhook handler to be more provider agnostic by adding to the provider interface methods for populating the initial event message (something similar to what we do when we build the entity wrapper?) but also have a method for processing a web hook event. Anyway, the PR is big enough so I'll approve to not block it 👍

Yes, however, I think we'll end up having provider-specific webhook implementations. I'd like it if we could figure out a more flexible way of handling this, but it'll have to wait for later.

This defines the provider implementations in such a way that we
can instantiate them and check if a rule type is able to accomodate for those.

This is done through a new `ProviderBuidler` implementation which is in
charge of instantiating the provider type that's needed at a given time.

Note that we still use the GitHub provider for HTTP calls since it has a lot
of handy features (such as rate limiting handling) that we could leverage.
@JAORMX
Copy link
Contributor Author

JAORMX commented Sep 26, 2023

@jhrozek fixed a merge conflict and resolved your suggestions.

@JAORMX JAORMX merged commit c1913aa into main Sep 26, 2023
12 checks passed
@JAORMX JAORMX deleted the providers-p2 branch September 26, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial providers implementation
3 participants