-
Notifications
You must be signed in to change notification settings - Fork 334
[WPB-21366] Refactor brig-index cli to use brig.yaml #4957
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
Conversation
0fdbd99 to
06df69c
Compare
manual testingApply this patch: Then create an "old" app without the types field in The "old" app is now generated, and
Note that there is no Damn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the brig-index CLI tool to use the main brig.yaml configuration file instead of requiring redundant CLI parameters. This change was necessary because brig-index now needs to access PostgreSQL, which would have made the CLI even more bloated with additional parameters.
Changes:
- Added a
--config-fileflag to brig-index that reads brig.yaml for connection settings (Elasticsearch, Cassandra, PostgreSQL, Galley) - Enabled the getUserType function to properly distinguish between regular users, bots, and app users by integrating AppStore access
- Updated all invocations of brig-index across tests, Makefile, integration scripts, and Helm charts to use the new config file approach
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/brig/index/src/Main.hs | Added config file parser and loading logic using --config-file flag |
| services/brig/src/Brig/Index/Eval.hs | Updated runCommand and runSem to accept Brig.Opts; added PostgreSQL pool initialization and AppStore interpreter |
| services/brig/src/Brig/Index/Options.hs | Added FUTUREWORK comment about further simplification opportunities |
| services/brig/test/integration/Index/Create.hs | Updated test calls to runCommand with brigOpts parameter |
| services/brig/test/integration/API/Search.hs | Updated test calls to runCommand with opts parameter |
| libs/wire-subsystems/src/Wire/IndexedUserStore/Bulk/ElasticSearch.hs | Uncommented and enabled getUserType implementation with AppStore access |
| integration/test/Test/Apps.hs | Enhanced tests to verify user types across team boundaries |
| integration/scripts/integration-dynamic-backends-brig-index.sh | Updated to use --config-file flag |
| charts/elasticsearch-index/templates/migrate-data.yaml | Added brig config/secrets volume mounts and --config-file argument |
| charts/elasticsearch-index/templates/create-index.yaml | Added brig config/secrets volume mounts and config file arguments |
| Makefile | Refactored es-reset to use --config-file and deduplicated db-migrate target |
| services/brig/brig.cabal | Added yaml dependency to brig-index executable |
| changelog.d/5-internal/brig-index-config-dedup | Added internal changelog entry |
| changelog.d/0-release-notes/WPB-21366-refactor-brig-index-cli | Added release notes about PostgreSQL requirement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b2a39fe to
1891b46
Compare
c7b862a to
f73e596
Compare
- new target "es-migrate" (must be separate from "es-reset" because to avoid cyclical dep with `make cr`) - "cr" target now uses "es-reset" as dependency - new target "postgres-migrate", make it dependency of "db-migrate" - fix phonyness annotation of "postgres-schema"
5d20fdc to
35c2c45
Compare
https://wearezeta.atlassian.net/browse/WPB-21366
brig-index needs to talk to postgres now. this PR adds
--config-file brig.yamlto its cli.this makes the already complicated cli even more complicated and also partially redundant. cleaning this up is left as FUTUREWORK.
This work is based on an earlier, failed attempt
Checklist
changelog.d