[Fleet] Add support for select type in integrations by jillguyonnet · Pull Request #152550 · elastic/kibana

jillguyonnet · 2023-03-02T10:34:29Z

Summary

This PR adds support for the new select var type in integrations.

Context and discussion in related issue: elastic/package-spec#453
Related package-spec PR: elastic/package-spec#486

Screenshots

Before

Current Scope advanced option in the Elasticsearch metrics settings (free text field):

After

Same option with a select instead:

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

…-ref HEAD~1..HEAD --fix'

elasticmachine · 2023-03-07T15:06:13Z

Pinging @elastic/fleet (Team:Fleet)

jillguyonnet · 2023-03-07T15:09:38Z

Hey @elastic/fleet , not sure which kind of release note this qualifies for, any suggestion?

kpollich · 2023-03-07T15:43:39Z

@jillguyonnet - This can probably use release_note:enhancement 🙂

nchaulet · 2023-03-07T20:11:47Z

This is great I am wondering if can add this to the package policy validation https://github.com/nchaulet/kibana/blob/290cb9497c34e73c498c58f2270834406cfa295c/x-pack/plugins/fleet/common/services/validate_package_policy.ts#L318-L339 and reject values that are not a select option.

jillguyonnet · 2023-03-08T09:04:31Z

...cy/create_package_policy_page/components/steps/components/package_policy_input_var_field.tsx

      isEditPage,
      isInvalid,
      fieldLabel,
+      options,


I was suspicious about memoizing an array since it would be compared by reference, but I'm not sure at all if that could be an issue in this case. I didn't see any strangeness with local testing. Does anyone know?

…-ref HEAD~1..HEAD --fix'

nchaulet · 2023-03-08T12:37:09Z

x-pack/plugins/fleet/common/services/validate_package_policy.ts

  }

+  if (varDef.type === 'select') {
+    if (!varDef.options) {


Should we validate that the value is one of the options instead? That service validate user provided value not the package structure.

Hey @nchaulet - Thanks for your input! Ah right, I suppose that will be handled by the package linter. Ok, so we can check that the value is either undefined or one the options' value.
Edit: I'm not sure in what scenario the value would be something else though?

Edit: I'm not sure in what scenario the value would be something else though?

For a package policy created via an API call user can provide any value they want

Thanks! I amended the validation, let me know.

…-ref HEAD~1..HEAD --fix'

nchaulet

LGTM 🚀

kibana-ci · 2023-03-08T14:48:11Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 3f950b7

Failed CI Steps

FTR Configs #50

Test Failures

[job] [logs] FTR Configs #50 / discover/group1 discover accessibility should give focus to the data view switch link when Tab is pressed

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`fleet`	989	990	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`fleet`	937.1KB	937.2KB	+113.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`fleet`	125.1KB	125.3KB	+262.0B

Unknown metric groups

API count

id	before	after	diff
`fleet`	1094	1095	+1

ESLint disabled line counts

id	before	after	diff
`securitySolution`	428	430	+2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	505	507	+2

History

💚 Build #112660 succeeded 647aad0
💔 Build #112622 failed a586480
💔 Build #112618 failed b2d5fce
💚 Build #112411 succeeded 3c9e055
💚 Build #111665 succeeded df2075eb14120977c185b46b1c661e2fbb75df15
💔 Build #111647 failed 58491a98531f47b9395827beaed2537e3cd51982

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

## Summary This PR adds support for the new `select` var type in integrations. Context and discussion in related issue: elastic/package-spec#453 Related package-spec PR: elastic/package-spec#486 ### Screenshots #### Before Current `Scope` advanced option in the Elasticsearch metrics settings (free text field): <img width="777" alt="Screenshot 2023-03-08 at 09 37 30" src="https://user-images.githubusercontent.com/23701614/223664474-a5cc5c2b-43c3-451f-9678-30e8c661bfcf.png"> #### After Same option with a select instead: <img width="777" alt="Screenshot 2023-03-08 at 09 36 25" src="https://user-images.githubusercontent.com/23701614/223664552-37c56bff-d6c9-4035-af4c-3a27e355beed.png"> <img width="777" alt="Screenshot 2023-03-08 at 09 36 31" src="https://user-images.githubusercontent.com/23701614/223664581-9e92abfa-6202-433e-b4de-2740fc7c96bc.png"> ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

## Summary This PR is a followup fix to #152550. As discovered in elastic/integrations#5451 (comment), there is currently an issue where the select is invalid upon first render because no default `value` exists, yet the first option appears to be selected. The fix introduced in this PR replaces the EUISelect with a more flexible EUIComboBox component, with a placeholder with text `Select an option` when no value is selected. In addition, this new component allows the selection to be cleared, which can be useful for non required variables. If the package specifies a default value, it should be selected by default. Otherwise, the placeholder will render. Note that the field will not be valid upon first render if the variable is required and no default value is provided. <img width="1728" alt="Screenshot 2023-03-29 at 17 47 28" src="https://user-images.githubusercontent.com/23701614/228596370-d6b14c03-4a09-4f68-a137-95bb7ca7e78f.png"> <img width="1728" alt="Screenshot 2023-03-29 at 17 47 35" src="https://user-images.githubusercontent.com/23701614/228596387-dc99e4b2-5d9d-4218-baf1-010bf527b028.png"> <img width="1728" alt="Screenshot 2023-03-29 at 17 47 44" src="https://user-images.githubusercontent.com/23701614/228596415-9f1f73af-a9f5-4a88-a700-999213500c4e.png"> ### Repro steps 1. Run package-registry with version 1.5.0 of Elasticsearch integration from elastic/integrations#5451: 1. In integrations repo, check out the branch in elastic/integrations#5451 2. In `packages/elasticsearch/changelog.yml`, change `version: 1.5.0-next` to `version: 1.5.0` 3. In `packages/elasticsearch/manifest.yml`, change the version to 1.5.0 4. In the shell, `cd packages/elasticsearch` if not there already and `elastic-package build -v` 5. Run the package registry: `elastic-package stack up -v -d --services package-registry` 6. Check that it is running correctly and that you can see version 1.5.0 of Elasticsearch at https://localhost:8080/search?package=elasticsearch 2. Run Kibana in dev on this branch: 1. In your `kibana.dev.yml`, add `xpack.fleet.registryUrl: https://localhost:8080` if not there (⚠️ https, not http) 2. Before running `yarn start`, run `export NODE_EXTRA_CA_CERTS=$HOME/.elastic-package/profiles/default/certs/kibana/ca-cert.pem` in the same shell. This is to set up the certificate needed to access EPR with https. 3. In Kibana, add the Elasticsearch integration (should be version 1.5.0). Check that the `Scope` setting is controlled by a select. Note: in the latest version, this integration now provides a default value, which should be set to `node`. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

jen-huang · 2023-04-06T19:31:37Z

Great work completing this @jillguyonnet. Can you leave a comment with testing instructions for the QA team (probably using the new version of the Elasticsearch integration)? (@amolnater-qasource @dikshachauhan-qasource)

jillguyonnet · 2023-04-11T09:31:55Z

@jen-huang @amolnater-qasource @dikshachauhan-qasource
Certainly! Here is a guide for testing this feature. Please let me know of any issues.

dikshachauhan-qasource · 2023-04-11T11:41:46Z

Hi @jillguyonnet

Thanks for sharing the guide. Further, we have validated the changes as mentioned are currently available on Cloud 8.8 Snapshot build and found them working fine.

Build details:

VERSION: 8.8.0
BUILD: 62018
COMMIT: caf4ef815627e3dae29d8d752567cede886c10f4

Observations:

Elasticsearch Integration version available is 1.6.1
Dropdown is available at Scope field with two values "Node" and "Cluster".
On de-selection or using cross option, user is unable to save the changes and validation messgae is displayed.

Further, when we are using Preview API request option to validate the changes, we observed improper validation message is displayed.

cc @jen-huang

Screenshot:

Please let us know if this is expected and if we missed any other validation.

Thanks
QAS

jillguyonnet · 2023-04-12T09:54:52Z

Hi @dikshachauhan-qasource

Thank you for your report!

Elasticsearch Integration version available is 1.6.1
Dropdown is available at Scope field with two values "Node" and "Cluster".
On de-selection or using cross option, user is unable to save the changes and validation messgae is displayed.

These are expected 👍

Further, when we are using Preview API request option to validate the changes, we observed improper validation message is displayed.

I can reproduce this as well with version 1.5.0 of Elasticsearch. We should perhaps investigate whether this issue is related to this change, I am having a look.
Edit: I can reproduce this issue on Kibana 8.6.1 and Elasticsearch 1.3.0. I would say this is not related to this change. cc @jen-huang

jen-huang · 2023-04-12T19:31:21Z

@jillguyonnet I agree it is not related to this change. @dikshachauhan-qasource could you file a separate issue with that finding? I think the policy_id field should not be present on that request in Preview API.

jen-huang · 2023-04-12T21:19:24Z

@jillguyonnet My mistake, the policy_id field should be present, but I guess it is not filled in automatically.

@dikshachauhan-qasource This is expected behavior. The Preview API content expects you to replace <agent_policy_id> with the id field from the first request.

dikshachauhan-qasource · 2023-04-13T12:02:39Z

Hi @jen-huang

Thanks for addressing above query and sharing your feedback. Further, as mentioned we have re-tried to validate above scenario using Preview API request and have following observations:

Working fine:

Scope field having Value "Node" in console, User is able to add integration successfully.

As per our understanding, Not Working Fine:

Cleared the scope value using cross, and used Preview API request button. On sending request from Console, user is able to add integration without any error.
Removed the scope variable from API request, still user able to add the integration successfully.
Provided null value in scope, here again user is able to add the integration successfully. Reported issue for this at [Fleet] User is able to add the Elasticsearch integration successfully when null value is provided in scope using Preview API request #154905

Screenshots:

Integration getting added to policy using API request:

Please let us know if we still are missing anything here.

Thanks
QAS

jen-huang · 2023-04-13T20:14:20Z

Removed the scope variable from API request, still user able to add the integration successfully.

@dikshachauhan-qasource This one is expected since this has a default value that will be filled in automatically if it is excluded: https://github.com/elastic/integrations/blob/main/packages/elasticsearch/manifest.yml#L73

Provided null value in scope, here again user is able to add the integration successfully

Will follow up on this in #154905.

dikshachauhan-qasource · 2023-04-14T09:10:24Z

This one is expected since this has a default value that will be filled in automatically if it is excluded: https://github.com/elastic/integrations/blob/main/packages/elasticsearch/manifest.yml#L73

Thank you @jen-huang for feedback on above mentioned scenario.

jillguyonnet mentioned this pull request Mar 7, 2023

[Change Proposal] Add support for "select/option" type for Integration parameters elastic/package-spec#453

Closed

jillguyonnet and others added 3 commits March 7, 2023 16:04

[Fleet] Add support for select type in integrations

798f4d5

Fix linting issues

1aac360

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

3c9e055

…-ref HEAD~1..HEAD --fix'

jillguyonnet force-pushed the jillguyonnet/fleet-add-select-integrations branch from df2075e to 3c9e055 Compare March 7, 2023 15:04

jillguyonnet marked this pull request as ready for review March 7, 2023 15:05

jillguyonnet requested a review from a team as a code owner March 7, 2023 15:05

jillguyonnet added the Team:Fleet Team label for Observability Data Collection Fleet team label Mar 7, 2023

jillguyonnet added the release_note:enhancement label Mar 7, 2023

nchaulet self-requested a review March 7, 2023 20:08

juliaElastic added the v8.8.0 label Mar 8, 2023

jillguyonnet added 3 commits March 8, 2023 09:29

Add options property to registry vars

1f53fa2

Use options category

b2d5fce

Fix linting error

d61c937

jillguyonnet commented Mar 8, 2023

View reviewed changes

kibanamachine and others added 4 commits March 8, 2023 09:07

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

a586480

…-ref HEAD~1..HEAD --fix'

Make options optional

6ba4907

Add validation test cases

717baae

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

647aad0

…-ref HEAD~1..HEAD --fix'

nchaulet reviewed Mar 8, 2023

View reviewed changes

jillguyonnet and others added 4 commits March 8, 2023 14:02

Add validation unit test

3f03a3f

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

116185e

…-ref HEAD~1..HEAD --fix'

Amend validation

df3ec99

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

3f950b7

…-ref HEAD~1..HEAD --fix'

nchaulet approved these changes Mar 8, 2023

View reviewed changes

jillguyonnet merged commit 9eb1c50 into elastic:main Mar 9, 2023

jillguyonnet deleted the jillguyonnet/fleet-add-select-integrations branch March 9, 2023 14:42

kibanamachine added the backport:skip This PR does not require backporting label Mar 9, 2023

This was referenced Mar 27, 2023

[Fleet] Make Elasticsearch scope setting a select elastic/integrations#5451

Merged

[Fleet] Add placeholder to integration select field #153927

Merged

jen-huang added the QA:Ready for Testing Code is merged and ready for QA to validate label Apr 6, 2023

amolnater-qasource removed the QA:Ready for Testing Code is merged and ready for QA to validate label Feb 23, 2024

Conversation

jillguyonnet commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Before

After

Checklist

For maintainers

Uh oh!

elasticmachine commented Mar 7, 2023

Uh oh!

jillguyonnet commented Mar 7, 2023

Uh oh!

kpollich commented Mar 7, 2023

Uh oh!

nchaulet commented Mar 7, 2023

Uh oh!

jillguyonnet Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

nchaulet Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

jillguyonnet Mar 8, 2023 • 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!

nchaulet Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

jillguyonnet Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

nchaulet left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Mar 8, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

jen-huang commented Apr 6, 2023

Uh oh!

jillguyonnet commented Apr 11, 2023

Uh oh!

dikshachauhan-qasource commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jillguyonnet commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jen-huang commented Apr 12, 2023

Uh oh!

jen-huang commented Apr 12, 2023

Uh oh!

dikshachauhan-qasource commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jen-huang commented Apr 13, 2023

Uh oh!

dikshachauhan-qasource commented Apr 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jillguyonnet commented Mar 2, 2023 •

edited

Loading

jillguyonnet Mar 8, 2023 •

edited

Loading

dikshachauhan-qasource commented Apr 11, 2023 •

edited

Loading

jillguyonnet commented Apr 12, 2023 •

edited

Loading

dikshachauhan-qasource commented Apr 13, 2023 •

edited

Loading