Skip to content

Comments

Fix federationStrategy 'allowAll'#3588

Merged
akshaymankar merged 9 commits intodevelopfrom
allow-all-take-2
Sep 26, 2023
Merged

Fix federationStrategy 'allowAll'#3588
akshaymankar merged 9 commits intodevelopfrom
allow-all-take-2

Conversation

@akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Sep 19, 2023

https://wearezeta.atlassian.net/browse/WPB-3796

Supersedes #3526

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 19, 2023
@akshaymankar akshaymankar force-pushed the allow-all-take-2 branch 2 times, most recently from 3138b00 to 9a00b23 Compare September 21, 2023 07:07
@akshaymankar akshaymankar force-pushed the allow-all-take-2 branch 4 times, most recently from 5b27017 to 0376d1b Compare September 26, 2023 07:46
@akshaymankar akshaymankar marked this pull request as ready for review September 26, 2023 08:09
@akshaymankar akshaymankar requested a review from fisx September 26, 2023 08:09
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm also ok with doing this in integration.cabal...

Copy link
Member Author

@akshaymankar akshaymankar Sep 26, 2023

Choose a reason for hiding this comment

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

Please create a PR then.

Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't called anywhere, but neither is it translated into /integration. i think running this won't pass for the same reason testHandleLookup fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleted it now.
The test already existed in the new test suite:

testRemoteUserSearch :: HasCallStack => App ()
testRemoteUserSearch = do
let overrides =
setField "optSettings.setFederationStrategy" "allowDynamic"
>=> setField "optSettings.setFederationDomainConfigsUpdateFreq" (Aeson.Number 1)
startDynamicBackends [def {brigCfg = overrides}, def {brigCfg = overrides}] $ \dynDomains -> do
domains@[d1, d2] <- pure dynDomains
connectAllDomainsAndWaitToSync 1 domains
[u1, u2] <- createAndConnectUsers [d1, d2]
Internal.refreshIndex d2
uidD2 <- objId u2
bindResponse (Public.searchContacts u1 (u2 %. "name") d2) $ \resp -> do
resp.status `shouldMatchInt` 200
docs <- resp.json %. "documents" >>= asList
case docs of
[] -> assertFailure "Expected a non empty result, but got an empty one"
doc : _ -> doc %. "id" `shouldMatch` uidD2

Copy link
Contributor

Choose a reason for hiding this comment

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

did you remove this search intentionally?

Copy link
Member Author

@akshaymankar akshaymankar Sep 26, 2023

Choose a reason for hiding this comment

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

Yes, the test is supposed to show that federation works in dynamic backends. Using search here is just making the test more complicated.

The tests require search policy to be set, it is easier to test with dynamic backends.
Now that allowAll is fixed and is the default for all test backends, we can
write tests in a simpler way.
Defaults to 5 min in the helm chart. For integration tests the value is set to
1s.
@akshaymankar akshaymankar merged commit d835b97 into develop Sep 26, 2023
@akshaymankar akshaymankar deleted the allow-all-take-2 branch September 26, 2023 10:06
@echoes-hq echoes-hq bot added the echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. 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.

3 participants