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

Changes index mapping api response to plural form #66012

Merged
merged 3 commits into from
May 13, 2020
Merged

Changes index mapping api response to plural form #66012

merged 3 commits into from
May 13, 2020

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented May 11, 2020 •
edited
Loading

This PR changes the API response from {mapping: ...} to {mappings: ...} in Index Management server code.

Fixes #59085.

Screenshot 2020-05-11 at 14 27 24

Release Note

We fixed mappings keyword in Index Management plugin (Index detail pane, Mapping tab).

@yuliacech yuliacech requested a review from a team as a code owner May 11, 2020 13:01
@yuliacech yuliacech added v7.7.1 v7.8.1 v7.9.0 v8.0.0 release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels May 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@yuliacech yuliacech added the Feature:Index Management Index and index templates UI label May 11, 2020
alisonelizabeth
alisonelizabeth reviewed May 11, 2020
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Nice work @yuliacech! Tested locally. Changes LGTM.

I like your suggestion here: #59085 (comment). I think it makes sense to update the tab as well. Do you mind fixing that as part of this PR?

@yuliacech
Copy link
Contributor Author

Hi @alisonelizabeth, thank you so much for your comment!
I updated the tab title to read "Mappings", however I was wondering if I could change the id value from xpack.idxMgmt.detailPanel.tabMappingLabel to xpack.idxMgmt.detailPanel.tabMappingsLabel in detail_panel.js:46?
I understand that I will then need to update the used id in ja-JP.json and zh-CN.json files. But will it not accidentally break some kind of translation workflow?

alisonelizabeth
alisonelizabeth approved these changes May 12, 2020
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @yuliacech for making the change! LGTM.

I think it’s OK to leave the i18n ID the same. If we change it, I believe it would require the translations team to translate it again. AFAIK we don’t typically manually edit the translation files. You can also run this by the kibana localization team to confirm 😄.

For future though, if you do need to remove an i18n string, you can run:
node scripts/i18n_check --fix to programmatically remove it. You can find more info on the i18n readme and guidelines pages.

@alisonelizabeth
Copy link
Contributor

@yuliacech Just noticed CI failed after I approved. It looks like you will also need to update the jest snapshot for the index_table tests to get them passing again.

The other test failure looks unrelated and you may just need to update your branch.

sebelga
sebelga approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

Changes LTGM! I haven't tested locally.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@yuliacech yuliacech merged commit fffe641 into elastic:master May 13, 2020
@yuliacech yuliacech deleted the fix-mappings-plural-form branch May 13, 2020 14:50
@yuliacech yuliacech mentioned this pull request May 13, 2020
Merged
yuliacech added a commit to yuliacech/kibana that referenced this pull request May 13, 2020
@yuliacech
[IM] Changes index mapping api response to plural form (elastic#66012)
3ab91fb 
* changes index mapping api response to plural form

* changes tab title to "Mappings" in Index Management detail panel

* updates index management jest snapshots for "Mappings" title
# Conflicts:
#	x-pack/test/api_integration/apis/management/index_management/mapping.js
@yuliacech yuliacech mentioned this pull request May 13, 2020
Merged
yuliacech added a commit to yuliacech/kibana that referenced this pull request May 13, 2020
@yuliacech
[IM] Changes index mapping api response to plural form (elastic#66012)
6d12eaf 
* changes index mapping api response to plural form

* changes tab title to "Mappings" in Index Management detail panel

* updates index management jest snapshots for "Mappings" title
# Conflicts:
#	x-pack/test/api_integration/apis/management/index_management/mapping.js
yuliacech added a commit to yuliacech/kibana that referenced this pull request May 13, 2020
@yuliacech
[IM] Changes index mapping api response to plural form (elastic#66012)
4a14576 
* changes index mapping api response to plural form

* changes tab title to "Mappings" in Index Management detail panel

* updates index management jest snapshots for "Mappings" title
# Conflicts:
#	x-pack/test/api_integration/apis/management/index_management/mapping.js
@yuliacech yuliacech mentioned this pull request May 13, 2020
Merged
yuliacech added a commit that referenced this pull request May 14, 2020
@yuliacech
[IM] Changes index mapping api response to plural form (#66012) (#66428)
2fd58fb 
* changes index mapping api response to plural form

* changes tab title to "Mappings" in Index Management detail panel

* updates index management jest snapshots for "Mappings" title
# Conflicts:
#	x-pack/test/api_integration/apis/management/index_management/mapping.js
yuliacech added a commit that referenced this pull request May 14, 2020
@yuliacech
[IM] Changes index mapping api response to plural form (#66012) (#66430)
5a7cefb 
* changes index mapping api response to plural form

* changes tab title to "Mappings" in Index Management detail panel

* updates index management jest snapshots for "Mappings" title
# Conflicts:
#	x-pack/test/api_integration/apis/management/index_management/mapping.js
yuliacech added a commit that referenced this pull request May 14, 2020
@yuliacech
[IM] Changes index mapping api response to plural form (#66012) (#66429)
9f3fc67 
* changes index mapping api response to plural form

* changes tab title to "Mappings" in Index Management detail panel

* updates index management jest snapshots for "Mappings" title
# Conflicts:
#	x-pack/test/api_integration/apis/management/index_management/mapping.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebelga sebelga sebelga approved these changes

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

Assignees
No one assigned
Labels
Feature:Index Management Index and index templates UI release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.7.1 v7.8.0 v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Typo in Index Management "mappings"
5 participants
@yuliacech @elasticmachine @alisonelizabeth @kibanamachine @sebelga