Move ui/indices into es_ui_shared plugin. by cjcenizal · Pull Request #60186 · elastic/kibana

cjcenizal · 2020-03-14T01:18:52Z

To test, ensure index name validation works in CCR auto-follow pattern and follower index creation forms, and the rollup job wizard. Ensure that help text still renders the disallowed characters \ / ? , " < > | * .

elasticmachine · 2020-03-14T01:18:54Z

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

cjcenizal · 2020-03-14T01:20:19Z

src/plugins/es_ui_shared/static/forms/helpers/field_validators/index_name.ts

-// Note: we can't import from "ui/indices" as the TS Type definition don't exist
-// import { INDEX_ILLEGAL_CHARACTERS_VISIBLE } from 'ui/indices';
+// @ts-ignore
+import { INDEX_ILLEGAL_CHARACTERS_VISIBLE } from '../../../../indices';


@sebelga AFAICT this file isn't consumed anywhere, so I wasn't sure how to test this. I don't want to spend time writing a test for it. I'm inclined to leave it with the expectation that a problem will be surfaced and fixed if/when we consume this file. What are your thoughts?

OK, although I think it would have been nice to make use of this single validator in our apps instead of re-writing it multiple times as I've seen. Maybe as a second pass 😊

cjcenizal · 2020-03-14T03:32:59Z

@elasticmachine merge upstream

lizozom

Code owner changes are only whitespaces

sebelga

Thanks for making those changes @cjcenizal ! I am concerned about 3 things

The folder structure of our es_ui_shared folder.

I am thinking of moving the forms and validators out of the static folder and move the content to either public either common folders.

I think we should only have 3 top-level folders "public" "server" and "common", like all plugins. I was surprised to see the console_lang as a top-level folder. It should be under a "console" folder in public.

Specific methods to validate indices

Now that we have generic validators, I think we should use them instead of adding the ones specific to indices. This means that from the "indices" folder we should probably only export the constants.

We should export everything from the top-level "public" or "server" folder.

I am surprised the linting does not complain about importing from other folders but for us, maintainers of the es_ui_shared folder, it makes our life much easier if we know that everything that other apps consume is exported from our 2 barrels files. This also means that we will probably need to namespace our exports

// es_ui_shared/public/index.ts

export const indices = {
  INDEX_ILLEGAL_CHARACTERS_VISIBLE,
};

Let me know what you think! 👍

sebelga · 2020-03-16T13:24:02Z

src/plugins/es_ui_shared/indices/index.d.ts

@@ -0,0 +1,26 @@
+/*


I think it would be better to rename the .js files to .ts instead of using a declaration file.

Also, I don't think we need those functions anymore as we have generic validator for each of them:

indexNameBeginsWithPeriod(value) --> startsWith('.')(value)

findIllegalCharactersInIndexName(value) --> containsChars(INDEX_ILLEGAL_CHARACTERS_VISIBLE)(value)

indexNameContainsSpaces(value) --> containsChars(' ')(value as string)

Can we use those instead?

sebelga · 2020-03-16T13:35:34Z

src/plugins/es_ui_shared/static/forms/helpers/field_validators/index_name.ts

-// Note: we can't import from "ui/indices" as the TS Type definition don't exist
-// import { INDEX_ILLEGAL_CHARACTERS_VISIBLE } from 'ui/indices';
+// @ts-ignore
+import { INDEX_ILLEGAL_CHARACTERS_VISIBLE } from '../../../../indices';


OK, although I think it would have been nice to make use of this single validator in our apps instead of re-writing it multiple times as I've seen. Maybe as a second pass 😊

sebelga · 2020-03-16T13:37:33Z

...k/legacy/plugins/cross_cluster_replication/public/app/components/auto_follow_pattern_form.js

 } from '@elastic/eui';

-import { INDEX_ILLEGAL_CHARACTERS_VISIBLE } from 'ui/indices';
+import { INDEX_ILLEGAL_CHARACTERS_VISIBLE } from '../../../../../../../src/plugins/es_ui_shared/indices';


Shouldn't we expose our static code from the public folder?

cjcenizal · 2020-03-17T05:26:42Z

@elasticmachine merge upstream

cjcenizal · 2020-03-17T05:28:05Z

Thanks for the review @sebelga! Great points. I moved the indices directory into public as you suggested and also converted everything to TS. I believe refactoring the code to use the generic validator is out of scope for this PR, as it would introduce unnecessary risk of regression, so I'd like to defer that to another PR. Could you take another look please?

sebelga · 2020-03-17T12:52:55Z

...s/cross_cluster_replication/public/app/components/follower_index_form/follower_index_form.js

  EuiTitle,
 } from '@elastic/eui';

+import { INDEX_ILLEGAL_CHARACTERS_VISIBLE } from '../../../../../../../../src/plugins/es_ui_shared/public';


It would be better to namespace our export.

import { indices } from '../../../../../../../../src/plugins/es_ui_shared/public; const { INDEX_ILLEGAL_CHARACTERS_VISIBLE } = indices;

This will avoid naming collisions and better organization of all our exports.

sebelga · 2020-03-17T12:53:38Z

...gacy/plugins/cross_cluster_replication/public/app/services/auto_follow_pattern_validators.js

  indexNameContainsSpaces,
-} from 'ui/indices';
-
+} from '../../../../../../../src/plugins/es_ui_shared/public';


Same here, it would be better to namespace under indices

cjcenizal · 2020-03-17T17:16:51Z

@sebelga Done. Could you re-re-review please?

sebelga

LGTM! Thanks for making the changes!

cjcenizal · 2020-03-18T00:44:08Z

@elasticmachine merge upstream

kibanamachine · 2020-03-18T02:27:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4059341

History

💔 Build #34017 failed 6bc97b3
💔 Build #33898 failed c33debddd43f779d7fb1872f5cbc617f1148aeb2
💔 Build #33703 failed 7294373
💚 Build #33269 succeeded 911640d
💔 Build #33245 failed c15fd1121c8766c815bbe6a7cdbba322d4dbeee5

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

* Convert js files to ts. * Add indices namespace.

* master: (35 commits) [Alerting] Adds navigation by consumer and alert type to alerting (elastic#58997) Introduce search interceptor (elastic#60523) [ML] Add functional tests for file data visualizer (elastic#60413) [APM] Optimize service map query (elastic#60412) [SIEM][Detection Engine] Adds lists feature flag and list values to the REST interfaces Enhancement/update esdocs datasource (elastic#59512) [junit] only include stdout in report for failures (elastic#60530) Update dependency nock to v12 (elastic#60422) upgrade execa to get stdout/stderr in error messages (elastic#60537) skip flaky suite (elastic#60471) [Ingest] Agent Config Details - Data sources list ui (elastic#60429) [SIEM] Create ML Rules (elastic#58053) skip flaky suite (elastic#60559) fix agent type (elastic#60554) Fixed default message for index threshold includes both threshold values (elastic#60545) [Ingest] Add support for `yaml` field types (elastic#60440) Solved the issue for a GROUP BY expression validation (elastic#60558) [Maps] Mark instance state as readonly (elastic#60557) Move ui/indices into es_ui_shared plugin. (elastic#60186) ServiceNow action improvements (elastic#60052) ...

* master: (64 commits) [Alerting] Adds navigation by consumer and alert type to alerting (elastic#58997) Introduce search interceptor (elastic#60523) [ML] Add functional tests for file data visualizer (elastic#60413) [APM] Optimize service map query (elastic#60412) [SIEM][Detection Engine] Adds lists feature flag and list values to the REST interfaces Enhancement/update esdocs datasource (elastic#59512) [junit] only include stdout in report for failures (elastic#60530) Update dependency nock to v12 (elastic#60422) upgrade execa to get stdout/stderr in error messages (elastic#60537) skip flaky suite (elastic#60471) [Ingest] Agent Config Details - Data sources list ui (elastic#60429) [SIEM] Create ML Rules (elastic#58053) skip flaky suite (elastic#60559) fix agent type (elastic#60554) Fixed default message for index threshold includes both threshold values (elastic#60545) [Ingest] Add support for `yaml` field types (elastic#60440) Solved the issue for a GROUP BY expression validation (elastic#60558) [Maps] Mark instance state as readonly (elastic#60557) Move ui/indices into es_ui_shared plugin. (elastic#60186) ServiceNow action improvements (elastic#60052) ...

Move ui/indices into es_ui_shared plugin.

6518c3c

cjcenizal added chore Feature:CCR and Remote Clusters v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes Feature:Rollups v7.7.0 labels Mar 14, 2020

cjcenizal requested a review from sebelga March 14, 2020 01:18

cjcenizal requested a review from a team as a code owner March 14, 2020 01:18

cjcenizal commented Mar 14, 2020

View reviewed changes

Fix TS errors.

911640d

cjcenizal force-pushed the chore/np-indices branch from c15fd11 to 911640d Compare March 14, 2020 18:19

lizozom approved these changes Mar 15, 2020

View reviewed changes

sebelga suggested changes Mar 16, 2020

View reviewed changes

alisonelizabeth mentioned this pull request Mar 16, 2020

[Cross Cluster Replication] NP Shim #60121

Merged

2 tasks

Move indices directory into public. Convert js files to ts.

0a61eb6

Merge branch 'master' into chore/np-indices

7294373

sebelga reviewed Mar 17, 2020

View reviewed changes

sebelga self-requested a review March 17, 2020 17:18

sebelga approved these changes Mar 17, 2020

View reviewed changes

Add indices namespace.

6bc97b3

cjcenizal force-pushed the chore/np-indices branch from c33debd to 6bc97b3 Compare March 17, 2020 22:17

Merge branch 'master' into chore/np-indices

4059341

cjcenizal merged commit 9aad898 into elastic:master Mar 18, 2020

cjcenizal deleted the chore/np-indices branch March 18, 2020 20:07

cjcenizal mentioned this pull request Mar 18, 2020

[7.x] Move ui/indices into es_ui_shared plugin. (#60186) #60561

Merged

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Mar 19, 2020

Move ui/indices into es_ui_shared plugin. (elastic#60186)

abe0a14

* Convert js files to ts. * Add indices namespace.

cjcenizal added a commit that referenced this pull request Mar 19, 2020

Move ui/indices into es_ui_shared plugin. (#60186) (#60561)

6feeda5

* Convert js files to ts. * Add indices namespace.

Conversation

cjcenizal commented Mar 14, 2020

Uh oh!

elasticmachine commented Mar 14, 2020

Uh oh!

cjcenizal Mar 14, 2020

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Mar 14, 2020

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Mar 17, 2020

Uh oh!

cjcenizal commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebelga Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Mar 17, 2020

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Mar 18, 2020

Uh oh!

kibanamachine commented Mar 18, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sebelga Mar 16, 2020 •

edited

Loading

sebelga Mar 16, 2020 •

edited

Loading

sebelga Mar 16, 2020 •

edited

Loading

cjcenizal commented Mar 17, 2020 •

edited

Loading