Skip to content

Comments

Brig Polysemy: Galley RPC effect#2653

Merged
mdimjasevic merged 22 commits intowireapp:developfrom
isovector:galley-rpc
Oct 10, 2022
Merged

Brig Polysemy: Galley RPC effect#2653
mdimjasevic merged 22 commits intowireapp:developfrom
isovector:galley-rpc

Conversation

@isovector
Copy link
Contributor

@isovector isovector commented Aug 25, 2022

This PR moves the majority of brig-to-galley's RPCs into a new GalleyProvider effect. In addition, it introduces two implementation-detail effects, ServiceRPC for routing RPCs to correct services, and RPC for actually sending the RPCs.

The vast majority of remaining MonadMask and MonadUnliftIO usages in brig correspond to RPCs. After doing the same treatment to cargohold and gundeck, it should hopefully be trivial to remove the MonadIO instance for AppT, at which point brig will be fully polysemized.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • I updated changelog.d subsections with one or more entries with the following bits of information (details):

@mdimjasevic mdimjasevic added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 26, 2022
@mdimjasevic mdimjasevic self-requested a review August 26, 2022 08:02
@mdimjasevic
Copy link
Contributor

I haven't looked into details, but you have a handful of conflicting files with the upstream develop branch.

@isovector
Copy link
Contributor Author

@mdimjasevic should be resolved now

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.

This looks great and I like how you defined the GalleyProvider interpreters! I will rebase my PR #2450 on top of this once it gets merged because my approach was direct (without the RPC and ServiceRPC effects) and in the PR I didn't turn into an effect everything Galley in the Brig.IO.Intra, but only what I needed to get the code compiling.

You have a TODO or two and one undefined function. Can you address them before we merge the PR?

@isovector isovector temporarily deployed to cachix October 3, 2022 16:00 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 16:00 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 18:49 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 18:49 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 19:12 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 19:12 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 19:26 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 19:26 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 19:28 Inactive
@isovector isovector temporarily deployed to cachix October 3, 2022 19:28 Inactive
@fisx
Copy link
Contributor

fisx commented Oct 4, 2022

potentially unpleasant question: would it be possible to skip this effect altogether, given we're planning to merge brig and galley soon, and it'll all just be function calls?

@isovector isovector temporarily deployed to cachix October 6, 2022 17:57 Inactive
@isovector isovector temporarily deployed to cachix October 6, 2022 17:57 Inactive
@isovector isovector temporarily deployed to cachix October 6, 2022 18:57 Inactive
@isovector isovector temporarily deployed to cachix October 6, 2022 18:57 Inactive
@isovector isovector temporarily deployed to cachix October 6, 2022 18:59 Inactive
@isovector isovector temporarily deployed to cachix October 6, 2022 18:59 Inactive
@mdimjasevic
Copy link
Contributor

@isovector , the PR fails with this in CI:

      TTL / Overrides

        increase to unlimited:                                                                                                                FAIL (0.38s)

          Error message: expected: FeatureTTLSeconds 2

           but got: FeatureTTLSeconds 1

          

          CallStack (from HasCallStack):

            assertFailure, called at ./Test/Tasty/HUnit/Orig.hs:86:32 in tasty-hunit-0.10.0.3-106eb1348444e345c4687ea268fe81c1fc84c38f18220dafc575b936429bc95c:Test.Tasty.HUnit.Orig

            assertEqual, called at ./Test/Tasty/HUnit/Orig.hs:108:23 in tasty-hunit-0.10.0.3-106eb1348444e345c4687ea268fe81c1fc84c38f18220dafc575b936429bc95c:Test.Tasty.HUnit.Orig

            @?=, called at test/integration/API/Teams/Feature.hs:504:18 in main:API.Teams.Feature

            getFeatureConfig, called at test/integration/API/Teams/Feature.hs:553:3 in main:API.Teams.Feature

            testSimpleFlagTTLOverride, called at test/integration/API/Teams/Feature.hs:94:44 in main:API.Teams.Feature

          Use -p '/increase to unlimited/' to rerun this test only.

I haven't looked at it, but it might be an unrelated flaky test. I'll re-run the CI to see if happens again.

@isovector
Copy link
Contributor Author

potentially unpleasant question: would it be possible to skip this effect altogether, given we're planning to merge brig and galley soon, and it'll all just be function calls?

Sorry, just saw this. It's doable, but is currently blocking getting more polysemy into brig. Assuming CI and review passes, the work here is done. Up to you though!

@mdimjasevic
Copy link
Contributor

There is another failure:

      delete team:                                                                     FAIL

        Exception: Assertions failed:

         1: 404 =/= 200

        

        Response was:

        

        Response {responseStatus = Status {statusCode = 200, statusMessage = "OK"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Fri, 07 Oct 2022 12:22:45 GMT"),("Server","Warp/3.3.17"),("Vary","Accept-Encoding"),("Content-Encoding","gzip"),("Content-Type","application/json;charset=utf-8")], responseBody = Just "{\"access\":[\"invite\"],\"access_role\":\"activated\",\"access_role_v2\":[\"team_member\",\"non_team_member\",\"service\"],\"creator\":\"23989a0d-4a23-4249-8975-87d0fa8e0831\",\"id\":\"88e5df0a-0a3d-460d-9686-2a7c863dbfdf\",\"last_event\":\"0.0\",\"last_event_time\":\"1970-01-01T00:00:00.000Z\",\"members\":{\"others\":[{\"conversation_role\":\"wire_admin\",\"id\":\"3dba3a8c-fe51-4ac6-a13b-dcda93c76d6a\",\"qualified_id\":{\"domain\":\"federation-test-helper.test-b5oqewkoxfih.svc.cluster.local\",\"id\":\"3dba3a8c-fe51-4ac6-a13b-dcda93c76d6a\"},\"service\":{\"id\":\"b5703907-b3f4-4cc8-9223-e58352f31faa\",\"provider\":\"c6c6bfc9-bb97-470c-bbd4-862c3f084215\"},\"status\":0},{\"conversation_role\":\"wire_admin\",\"id\":\"b3675ca5-3fdf-4a59-9385-2edbfb013051\",\"qualified_id\":{\"domain\":\"federation-test-helper.test-b5oqewkoxfih.svc.cluster.local\",\"id\":\"b3675ca5-3fdf-4a59-9385-2edbfb013051\"},\"status\":0}],\"self\":{\"conversation_role\":\"wire_admin\",\"hidden\":false,\"hidden_ref\":null,\"id\":\"23989a0d-4a23-4249-8975-87d0fa8e0831\",\"otr_archived\":false,\"otr_archived_ref\":null,\"otr_muted_ref\":null,\"otr_muted_status\":null,\"qualified_id\":{\"domain\":\"federation-test-helper.test-b5oqewkoxfih.svc.cluster.local\",\"id\":\"23989a0d-4a23-4249-8975-87d0fa8e0831\"},\"service\":null,\"status\":0,\"status_ref\":\"0.0\",\"status_time\":\"1970-01-01T00:00:00.000Z\"}},\"message_timer\":null,\"name\":null,\"protocol\":\"proteus\",\"qualified_id\":{\"domain\":\"federation-test-helper.test-b5oqewkoxfih.svc.cluster.local\",\"id\":\"88e5df0a-0a3d-460d-9686-2a7c863dbfdf\"},\"receipt_mode\":null,\"team\":\"115bf155-b5b7-4099-b5ff-473d1eb65b5a\",\"type\":0}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}

        CallStack (from HasCallStack):

          error, called at src/Bilge/Assert.hs:89:5 in bilge-0.22.0-inplace:Bilge.Assert

          <!!, called at src/Bilge/Assert.hs:107:19 in bilge-0.22.0-inplace:Bilge.Assert

          !!!, called at test/integration/API/Provider.hs:737:7 in main:API.Provider

        Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&/delete team/' to rerun this test only.

@mdimjasevic
Copy link
Contributor

@fisx , I find this a useful PR. Before we can merge Brig and Galley, we have to fully polysemise Brig, and this PR is bringing us in that direction.

GetExposeInvitationURLsToTeamAdmin id' -> getTeamExposeInvitationURLsToTeamAdmin id'

runIt :: HttpClientIO a -> Sem r a
runIt = undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct?

@mdimjasevic mdimjasevic temporarily deployed to cachix October 10, 2022 09:38 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix October 10, 2022 09:38 Inactive
@mdimjasevic
Copy link
Contributor

There is another failure:

      delete team:                                                                     FAIL
> ```

I couldn't reproduce this once locally, and neither could @elland . I believe it's a flaky test. So re-running it once again in CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants