-
Notifications
You must be signed in to change notification settings - Fork 30
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
[Console] Update format of services.yaml #718
[Console] Update format of services.yaml #718
Conversation
Signed-off-by: Mikayla Thompson <thomika@amazon.com>
Signed-off-by: Mikayla Thompson <thomika@amazon.com>
Signed-off-by: Mikayla Thompson <thomika@amazon.com>
Signed-off-by: Mikayla Thompson <thomika@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #718 +/- ##
============================================
- Coverage 63.29% 62.96% -0.33%
Complexity 1579 1579
============================================
Files 220 231 +11
Lines 9156 9729 +573
Branches 793 793
============================================
+ Hits 5795 6126 +331
- Misses 2950 3195 +245
+ Partials 411 408 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mikayla Thompson <thomika@amazon.com>
Signed-off-by: Mikayla Thompson <thomika@amazon.com>
@@ -17,33 +17,32 @@ For example: | |||
|
|||
```yaml | |||
source_cluster: | |||
endpoint: "https://capture-proxy-es:9200" |
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.
Thanks!
@@ -12,38 +13,47 @@ | |||
AuthMethod = Enum("AuthMethod", ["NO_AUTH", "BASIC_AUTH", "SIGV4"]) | |||
HttpMethod = Enum("HttpMethod", ["GET", "POST", "PUT", "DELETE"]) | |||
|
|||
|
|||
NO_AUTH_SCHEMA = { |
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.
nit: Do you have a link you can comment for what schema options like "nullable" are available for a schema
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.
Description
The services.yaml file is the core location for information about the current deployment and making the implementation clean and easy to validate will be helpful moving forward.
There were a few recent comments regarding the file format in the reviews of the e2e branch merge & the OSI backfill additions:
For the second point, the idea is to move AWAY from:
And towards:
In general, instead of having a type field, each service description should instead have a node with the name of the type that contains any relevant details. The schema validation can then be written to ensure that no mutually-exclusive nodes are present.
Notes on Approach
The bulk of this PR is updating documentation and tests--the actual schemas involved are quite small still (hence doing this refactoring now).
There was no built-in Cerberus rule to validate that exactly one block of a selection was present. I created a generator that returns a callable to verify that exactly one key from a given list is provided. I think this should be quite widely used throughout our schemas. I also think that this PR makes the schemas a bit more modularized and readable.
Issues Resolved
https://opensearch.atlassian.net/browse/MIGRATIONS-1754
Testing
Unit tests, manual
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.