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

Update API Specs for dev console #2226

Merged
merged 2 commits into from
Sep 8, 2022

Conversation

kristenTian
Copy link
Contributor

@kristenTian kristenTian commented Aug 30, 2022

Signed-off-by: Kristen Tian [email protected]

Description

Update API Specs for dev console following SOP: https://cptnb.github.io/opensearch-dashboards-dev-docs/core-plugins/console/console/

  • Reminder to keep the opensearch documents until 4350 is addressed.

Issues Resolved

#2219

Check List

  • Commits are signed per the DCO using --signoff

joshuarrrr
joshuarrrr previously approved these changes Aug 31, 2022
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Nice, this seems like a big win. Only one note about future enhancements to the generation script.

"osd",
"kb",
Copy link
Member

Choose a reason for hiding this comment

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

This 💯 looked like a regression to me, and then I took a second look at what this array is... 😂

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it was ha.

@@ -15,6 +15,8 @@
"p",
"pb"
],
"master_timeout": "",
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope for this PR, but as a note, we may want to update the generation script to ignore deprecated properties, as a more subtle way of guiding folks to the new usage. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe for our side we can just move this out. We can set up some mechanism but good catch @joshuarrrr!. @kristenTian can we delete line 18 due to it not being inclusive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will delete all master_timeout under src/plugins/console/server/lib/spec_definitions/json/generated path

@joshuarrrr
Copy link
Member

Can you also update the commit message to be more explanatory about how this changeset was generated? It's just a helpful breadcrumb for the next dev that needs to do this.

"indices.add_block": {
"url_params": {
"timeout": "",
"master_timeout": "",
Copy link
Member

@kavilla kavilla Aug 31, 2022

Choose a reason for hiding this comment

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

same for this can we just remove this master_timeout?

"indices.delete_index_template": {
"url_params": {
"timeout": "",
"master_timeout": "",
Copy link
Member

Choose a reason for hiding this comment

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

same remove master_timeout

"indices.exists_index_template": {
"url_params": {
"flat_settings": "__flag__",
"master_timeout": "",
Copy link
Member

Choose a reason for hiding this comment

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

same remove master_timeout

"indices.get_index_template": {
"url_params": {
"flat_settings": "__flag__",
"master_timeout": "",
Copy link
Member

Choose a reason for hiding this comment

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

same remove master_timeout

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

For the sake of being inclusive, lets just remove the master_timeout

"url_params": {
"create": "__flag__",
"cause": "",
"master_timeout": "",
Copy link
Member

Choose a reason for hiding this comment

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

same remove master_timeout

"url_params": {
"create": "__flag__",
"cause": "",
"master_timeout": "",
Copy link
Member

Choose a reason for hiding this comment

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

same remove master_timeout

"url_params": {
"create": "__flag__",
"cause": "",
"master_timeout": "",
Copy link
Member

Choose a reason for hiding this comment

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

same remove master_timeout

{
"snapshot.clone": {
"url_params": {
"master_timeout": "",
Copy link
Member

Choose a reason for hiding this comment

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

same remove master_timeout

@kristenTian
Copy link
Contributor Author

Can you also update the commit message to be more explanatory about how this changeset was generated? It's just a helpful breadcrumb for the next dev that needs to do this.

added the SOP/doc

@codecov-commenter
Copy link

Codecov Report

Merging #2226 (8537e72) into main (65005be) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 8537e72 differs from pull request most recent head 90d0ddf. Consider uploading reports for the commit 90d0ddf to get more accurate results

@@            Coverage Diff             @@
##             main    #2226      +/-   ##
==========================================
- Coverage   67.21%   67.20%   -0.02%     
==========================================
  Files        3100     3100              
  Lines       59579    59579              
  Branches     9064     9064              
==========================================
- Hits        40049    40043       -6     
- Misses      17344    17349       +5     
- Partials     2186     2187       +1     
Impacted Files Coverage Δ
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 83.67% <0.00%> (-4.09%) ⬇️
packages/osd-optimizer/src/node/cache.ts 50.00% <0.00%> (-2.78%) ⬇️
...ared/static/forms/hook_form_lib/hooks/use_field.ts 65.70% <0.00%> (-0.97%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

LGTM!

@kavilla
Copy link
Member

kavilla commented Sep 8, 2022

Thank you!

@kavilla kavilla merged commit 7f71c87 into opensearch-project:main Sep 8, 2022
@ananzh ananzh added the v3.0.0 label Sep 8, 2022
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Sep 14, 2022
* Update API Specs for dev console
* Remove deprecated field - master_timeout

Signed-off-by: Kristen Tian <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
* Update API Specs for dev console
* Remove deprecated field - master_timeout

Signed-off-by: Kristen Tian <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants