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

Migrate /translations route to core #83280

Merged
merged 3 commits into from
Nov 16, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Nov 12, 2020

Summary

Follow-up of #81799

Migrate the /translations/{locale}.json route to core.

Checklist

@pgayvallet pgayvallet added Feature:Legacy Removal Issues related to removing legacy Kibana v7.11 v8.0.0 labels Nov 12, 2020
Comment on lines +149 to +150
// setup i18n prior to any other service, to have translations ready
const i18nServiceSetup = await this.i18n.setup({ http: httpSetup, pluginPaths });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to move this after the http service setup to register the route. I could move i18n initialization in another method (like we did for plugin.discover) to keep the i18n setup before context and http, but I don't think this is really necessary

Comment on lines +22 to +26
export default function ({ getService }: FtrProviderContext) {
const supertest = getService('supertest');

describe('compression', () => {
it(`uses compression when there isn't a referer`, async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted / converted to TS from test/api_integration/apis/core/index.js

Comment on lines +60 to +65
headers: {
'content-type': 'application/json',
'cache-control': 'must-revalidate',
etag: translationCache.hash,
},
body: translationCache.translations,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed with @restrry, we ideally should use cache-control instead of etag here in dist mode to avoid an unnecessary roundtrip. However as this requires some refactoring to be able to add a path prefix,suffix or query param to invalidate the request when the kibana version is bumped (as we do with static assets), it will be done at a later time. I will create an issue for that before the current PR got merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #83409

@pgayvallet pgayvallet marked this pull request as ready for review November 12, 2020 16:06
@pgayvallet pgayvallet requested a review from a team as a code owner November 12, 2020 16:06
@pgayvallet pgayvallet added v7.11.0 release_note:skip Skip the PR/issue when compiling release notes and removed v7.11 labels Nov 12, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 42828 42830 +2
oss 22473 22475 +2

History

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

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

tested locally, looks good

@pgayvallet pgayvallet merged commit d1abc86 into elastic:master Nov 16, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Nov 16, 2020
* move i18n route to core

* add FTR test for endpoint
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 16, 2020
* master:
  Migrate `/translations` route to core (elastic#83280)
  [APM] Ensure APM jest script can run (elastic#83398)
  [Uptime] Monitor status alert use url as instance (elastic#81736)
  [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259)
  TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964)
  [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139)
  Skips Vega test
  skip flaky suite (elastic#79967)
  [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020)
  Added ability to fire actions when an alert instance is resolved (elastic#82799)
  [ML] Adds functional tests for the index data visualizer card contents (elastic#83174)
pgayvallet added a commit that referenced this pull request Nov 16, 2020
* move i18n route to core

* add FTR test for endpoint
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 16, 2020
… into add-logs-to-node-details

* 'add-logs-to-node-details' of github.com:phillipb/kibana:
  fix tall vislib charts in visualize (elastic#83340)
  [Lens] Avoid unnecessary data fetching on dimension flyout open (elastic#82957)
  [Security Solution][Case] Change case connector minimum required license to basic (elastic#83401)
  fix logstash central pipeline management test  (elastic#83281)
  [Search] Send to background UI (elastic#81793)
  Migrate `/translations` route to core (elastic#83280)
  [APM] Ensure APM jest script can run (elastic#83398)
  [Uptime] Monitor status alert use url as instance (elastic#81736)
  [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259)
  TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964)
  [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139)
  Skips Vega test
  skip flaky suite (elastic#79967)
  [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020)
  Added ability to fire actions when an alert instance is resolved (elastic#82799)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Legacy Removal Issues related to removing legacy Kibana release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants