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.

Sign up for GitHub

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

Jump to bottom

WPB-6717 ES Credentials #3959

Merged
merged 37 commits into from
Mar 25, 2024
Merged

WPB-6717 ES Credentials #3959

merged 37 commits into from
Mar 25, 2024

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Mar 15, 2024 •
edited
Loading

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 Mar 15, 2024
@battermann battermann marked this pull request as ready for review March 19, 2024 14:24
@battermann battermann force-pushed the WPB-6717-implement-credential-support-in-wires-elastic-search-interface branch from 51a8380 to 4d8fb4d Compare March 20, 2024 07:32
@supersven supersven self-requested a review March 20, 2024 07:45
@smatting
Copy link
Contributor

Shouldn't this be reflected in the docs somewhere? For example in the brig chart it's not obvious in what format .elasticsearch has to be, or that this parameter even exists.

@battermann
Copy link
Contributor Author

Shouldn't this be reflected in the docs somewhere? For example in the brig chart it's not obvious in what format .elasticsearch has to be, or that this parameter even exists.

Absolutely, I'll add it.

@battermann @supersven
Update docs/src/developer/reference/config-options.md
6589704 
Co-authored-by: Sven Tennie <sven.tennie@wire.com>
@battermann battermann requested a review from supersven March 20, 2024 09:02
supersven
supersven approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@supersven supersven left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Only a few nitpicks.

akshaymankar
akshaymankar reviewed Mar 25, 2024
View reviewed changes
services/brig/src/Brig/Options.hs Outdated
Comment on lines 628 to 630
setElasticsearchCredentials :: !(Maybe FilePathSecrets),
-- | Credentials for additional ES index (maily used for migrations)
setElasticsearchAdditionalCredentials :: !(Maybe FilePathSecrets)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not part of ElasticSearchOpts?

services/brig/test/resources/elasticsearch-credentials.yaml
Comment on lines +1 to +2
username: "elastic"
password: "changeme"
Copy link
Member

Choose a reason for hiding this comment

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

It feels wrong to have this file because for RabbitMQ we have environment variables. We should be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed in chat. there are other secrets which are configured in files. So we're already not consistent.
To avoid spending more time on this, we can make sure the helm charts are configured in a way that ensures we can change our minds on this later.

@battermann battermann merged commit d068678 into develop Mar 25, 2024
8 checks passed
@battermann battermann deleted the WPB-6717-implement-credential-support-in-wires-elastic-search-interface branch March 25, 2024 14:16
@elland elland mentioned this pull request Apr 25, 2024
Merged
@echoes-hq echoes-hq bot added echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... echoes: technical-roadmap/security More specific category, to highlight task that tackle security requirements. labels Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supersven supersven supersven approved these changes

@akshaymankar akshaymankar akshaymankar approved these changes

Assignees
No one assigned
Labels
echoes: technical-roadmap/security More specific category, to highlight task that tackle security requirements. echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@battermann @smatting @akshaymankar @supersven @zebot