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

MM-18068 Route integration actions to plugins without hitting the network #12156

Merged
merged 32 commits into from
Nov 6, 2019
Merged

MM-18068 Route integration actions to plugins without hitting the network #12156

merged 32 commits into from
Nov 6, 2019

Conversation

scottleedavis
Copy link
Contributor

@scottleedavis scottleedavis commented Sep 11, 2019

Summary

Routes integration actions to plugins without hitting the network and instead calls hooks.ServeHTTP directly

Ticket Link

Fixes #11964

@hanzei hanzei added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Sep 11, 2019
@hanzei
Copy link
Contributor

hanzei commented Sep 23, 2019

@scottleedavis Head up: @crspeller was out last week and will be out this week. And @lieut-data is a bit overloaded with reviews. Hence, it will take some time to review this. Sorry for the delay.

@hanzei
Copy link
Contributor

hanzei commented Sep 23, 2019

@gabrieljackson Can you help and jump in?

@scottleedavis
Copy link
Contributor Author

No worries @hanzei . I am happy to see this through to completion regardless of delay.

Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

Thanks for taking a stab at this, @scottleedavis! Comments below :)

app/integration_action.go Outdated Show resolved Hide resolved
app/integration_action_test.go Outdated Show resolved Hide resolved
app/integration_action.go Outdated Show resolved Hide resolved
app/integration_action.go Outdated Show resolved Hide resolved
app/integration_action.go Show resolved Hide resolved
@lieut-data lieut-data self-requested a review September 30, 2019 01:14
Copy link
Contributor

@gabrieljackson gabrieljackson left a comment

Choose a reason for hiding this comment

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

This is coming along nicely! A few comments below.

app/integration_action_test.go Outdated Show resolved Hide resolved
app/integration_action.go Outdated Show resolved Hide resolved
Copy link
Member

@lieut-data lieut-data 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, @scottleedavis! Just one change requested in the unit tests below.

app/integration_action_test.go Outdated Show resolved Hide resolved
@scottleedavis
Copy link
Contributor Author

gentle reminder for review @DHaussermann

@scottleedavis
Copy link
Contributor Author

test failures involve node_modules and redux, ergo I believe is not related to this PR.

@DHaussermann DHaussermann added the Setup Cloud Test Server Setup an on-prem test server label Oct 31, 2019
@DHaussermann
Copy link

@scottleedavis or @lieut-data Can you please provide some guidance or test steps on this? Can I verify this on the Spinwick server by watching client-side web traffic? I'm not sure I fully understand the problem being solved.

@lieut-data
Copy link
Member

@DHaussermann, previously, when an integration action was posted to a plugin, the flow was:

client -> server -> http request to http://site_url -> server -> plugin

This created problems whenever the server couldn't actually connect to its own site url for any number of reasons. Now, the behaviour is effectively:

client -> server -> plugin

since the server realizes it's a relative URL and skips the network lookup altogether.

It may be possible to test this on the spinwick by setting a bogus SiteURL.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Regression tested by ensuring post actions still work. Used Giphy and Matterpoll to confirm.
LGTM!

@DHaussermann DHaussermann removed the 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review label Nov 5, 2019
@hanzei hanzei added Awaiting Submitter Action Blocked on the author 4: Reviews Complete All reviewers have approved the pull request labels Nov 5, 2019
@hanzei
Copy link
Contributor

hanzei commented Nov 5, 2019

@scottleedavis Can you please merge master into this? Then we can merge.

@scottleedavis
Copy link
Contributor Author

complete. build test failures look unrelated.

@hanzei hanzei self-assigned this Nov 6, 2019
@lieut-data lieut-data removed Awaiting Submitter Action Blocked on the author Setup Cloud Test Server Setup an on-prem test server labels Nov 6, 2019
@mattermod
Copy link
Contributor

Test server destroyed

@lieut-data
Copy link
Member

Indeed, the CircleCI errors seem specious. The required tests have passed, so I think we're good here: merging. And thanks, @scottleedavis, as always :)

@lieut-data lieut-data merged commit 516017d into mattermost:master Nov 6, 2019
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Nov 18, 2019
@hanzei hanzei removed their assignment Sep 25, 2020
@boredomdenied
Copy link

I seem to be having an issue related to this in some way. Would you mind taking a look at the issue mentioned above @scottleedavis Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Route integration actions to plugins without hitting the network
9 participants