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.

Sign up for GitHub

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

Jump to bottom

Refactor webhook route config #243

Merged
merged 12 commits into from
Mar 21, 2019
Merged

Refactor webhook route config #243

merged 12 commits into from
Mar 21, 2019

Conversation

VaibhavPage
Copy link
Contributor

@VaibhavPage VaibhavPage commented Mar 20, 2019 •
edited
Loading

  1. Get rid of Configs map from RouteConfig. Make RouteConfig as an interface.
  2. Fix github gateway issue introduced by gateway/github: Enterprise support and some fixes #221

This PR does not contain any breaking changes to spec.

@VaibhavPage
Copy link
Contributor Author

@magaldima @dtaniwaki please review the pr.

@VaibhavPage VaibhavPage mentioned this pull request Mar 20, 2019
Closed
@dtaniwaki
Copy link
Member

Get rid of Configs map from RouteConfig. Make RouteConfig as an interface.

@VaibhavPage Could you tell me the motivation of this interface change to start reviewing?

Fix github gateway issue introduced by #221

By the way, I think this fix should've been separated for easier review and future reference. I prefer keeping PRs as small and independent as possible.

@VaibhavPage
Copy link
Contributor Author

VaibhavPage commented Mar 20, 2019 •
edited
Loading

Yeah, PRs have been big for a while, I will try to break these in smaller pieces in future 😄

In webhook like gateways, we had RouteConfig which was a map basically used to hold gateway specific interfaces that are separate from common webhook fields. So to extract the values out of RouteConfig, we had to cast these (possibility for nil pointer) interfaces into concrete structs and was needed to maintain label keys so that we can store these interfaces against them.

Making RouteConfig as interface now mandates all webhook like gateways to implement it and the implementing struct can contain the extra fields which now can be accessed directly in the methods like RouteHandler.

The issue introduced by #221 - The code didn't check whether hook from CreateWebhook was nil and it was trying to access fields on it.

@VaibhavPage VaibhavPage marked this pull request as ready for review March 20, 2019 15:31
dtaniwaki
dtaniwaki reviewed Mar 20, 2019
View reviewed changes
Copy link
Member

@dtaniwaki dtaniwaki left a comment

Choose a reason for hiding this comment

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

Commented about the interface name.

@VaibhavPage VaibhavPage requested a review from dtaniwaki March 20, 2019 18:07
dtaniwaki
dtaniwaki previously approved these changes Mar 21, 2019
View reviewed changes
Copy link
Member

@dtaniwaki dtaniwaki left a comment

Choose a reason for hiding this comment

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

LGtM!

@VaibhavPage VaibhavPage requested a review from spk83 March 21, 2019 16:11
@spk83 spk83 requested a review from dtaniwaki March 21, 2019 16:13
@VaibhavPage VaibhavPage merged commit 5084e6d into master Mar 21, 2019
This was referenced Mar 21, 2019
Closed
Closed
@VaibhavPage VaibhavPage deleted the refactor-webhook-route-config branch March 31, 2019 02:50
juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022
@VaibhavPage
Refactor webhook route config (argoproj#243)
1effc18 
* refactor(): webhook route config

* refactor(): webhook route config and removing trello gateway

* refactor(): route config

* fix(): bug introduced from argoproj#221

* chore(): fix calendar gateway example

* fix(): sns gateway test

* test(): recover test

* fix(): makefile

* chore(): updating comments

* chore(): updating makefile image version

* refactor(): rename RouteConfig interface to RouteManager
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spk83 spk83 spk83 approved these changes

@magaldima magaldima Awaiting requested review from magaldima

@dtaniwaki dtaniwaki Awaiting requested review from dtaniwaki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@VaibhavPage @dtaniwaki @spk83