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

federator: rename Brig -> Service and add galley #1570

Merged
merged 5 commits into from
Jun 7, 2021

Conversation

pcapriotti
Copy link
Contributor

Federator is currently forwarding all its requests to brig, regardless of the value of the Component field. This PR adds support for arbitrary internal services, renames the Brig effect to Service to reflect its generalised function, and adds a configuration option for galley.

@pcapriotti pcapriotti force-pushed the pcapriotti/federator-galley branch 2 times, most recently from ac1152a to bf4c341 Compare June 3, 2021 14:32
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Hm, interesting! We didn't catch this before, when we only use mocks... but it would be nice to be able to catch this sort of problem without an end2end test.

As https://github.com/wireapp/wire-server/blob/develop/services/brig/test/integration/Federation/End2end.hs#L236 returns a 200, if galley can't be called yet, I suppose that is because currently only a call to a remote brig is made, and #1556 isn't merged yet?

(Even though this is only a draft, changes so far look good)

@pcapriotti
Copy link
Contributor Author

Hm, interesting! We didn't catch this before, when we only use mocks... but it would be nice to be able to catch this sort of problem without an end2end test.

Agreed, but I'm not sure how.

As https://github.com/wireapp/wire-server/blob/develop/services/brig/test/integration/Federation/End2end.hs#L236 returns a 200, if galley can't be called yet, I suppose that is because currently only a call to a remote brig is made, and #1556 isn't merged yet?

That's right, that test is indeed failing in #1556.

@pcapriotti pcapriotti marked this pull request as ready for review June 4, 2021 15:24
@pcapriotti pcapriotti force-pushed the pcapriotti/federator-galley branch from 0620cb8 to 0e7a79d Compare June 7, 2021 04:52
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Overall this looks good. The only thing I am not sure about is the implementation of interpretService. @pcapriotti , can you tell if it needs updating?

@pcapriotti
Copy link
Contributor Author

Thanks for the review. I changed whatever brig name was left into service. I also updated the label passed to rpc with the actual name of the service, and added a test for galley requests.

Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Great!

I left a tiny comment in a Galley test you added.


res :: InwardResponse <- mock @Service @IO . noLogs . Polysemy.runReader allowAllSettings $ callLocal request
actualCalls <- mockServiceCallCalls @IO
let expectedCall = (Galley, "/users", "\"foo\"", aValidDomain)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this well, but shouldn't this be about a Galley-related endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it should. It's just a mocked request, so it doesn't matter, but I'll turn it into a galley endpoint for clarity.

@pcapriotti pcapriotti merged commit a427505 into develop Jun 7, 2021
@pcapriotti pcapriotti deleted the pcapriotti/federator-galley branch June 7, 2021 13:15
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.

4 participants