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

Strongly typed Datastore configuration - Fixes #2088 #2223

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

johnaohara
Copy link
Member

This is a fairly major re-factor of the Datastore configuration API.

It solves the following problems;

  • Add Strongly Typed authentication models: No Auth, Api Key and Username/Password
  • Add openapi discriminators for datastore authentication types
  • Fixes validation for all datastores: Fixes Elasticsearch configuration always requires credentials #2088
  • UI dynamically displays auth properties based on available auth types and user selected auth type
  • Add validation of authentication when updated existing datastores

@johnaohara johnaohara changed the title Issue 2088 Strongly typed Datastore configuration - Fixes #2088 Dec 24, 2024
@johnaohara johnaohara marked this pull request as ready for review December 24, 2024 09:20
@johnaohara johnaohara marked this pull request as draft December 24, 2024 09:21
@johnaohara johnaohara marked this pull request as ready for review January 2, 2025 15:31
@johnaohara johnaohara requested review from barreiro and lampajr January 2, 2025 15:31
Copy link
Collaborator

@barreiro barreiro left a comment

Choose a reason for hiding this comment

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

I added several comments to the code, but overall looks good.

@johnaohara johnaohara force-pushed the issue_2088 branch 3 times, most recently from 94aecb4 to de69cbb Compare January 3, 2025 15:49
Copy link
Collaborator

@barreiro barreiro left a comment

Choose a reason for hiding this comment

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

Added some suggestions for more cleanup on the API module

Copy link
Member

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

LGTM!

I added just a minor comment, up to you to include it or not as it would be just a simplification 🚀

- Updates from review
- Update datastore config checks touse isBlank()
- Update docs
@barreiro
Copy link
Collaborator

barreiro commented Jan 8, 2025

I think this one is good to go.

@barreiro barreiro merged commit 5925c2f into Hyperfoil:master Jan 8, 2025
2 checks passed
@lampajr lampajr mentioned this pull request Jan 8, 2025
4 tasks
@johnaohara johnaohara deleted the issue_2088 branch January 8, 2025 17:04
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.

Elasticsearch configuration always requires credentials
4 participants