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

[Automatic Import] rename plugin to automatic import #207325

Merged

Conversation

haetamoudi
Copy link
Contributor

@haetamoudi haetamoudi commented Jan 21, 2025

Summary

Rename integration-assistant plugin to automatic-import. Github issue

Outdated PR to rename plugin,

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Commands executed

  • node scripts/i18n_check --fix
  • node scripts/lint_ts_projects
  • node scripts/generate codeowners
  • node scripts/build_plugin_list_docs
  • node scripts/build_kibana_platform_plugins.js --update-limits

Testing

  • cypress tests:
    • yarn cypress:run --spec cypress/e2e/privileges_integrations_automatic_import_license.cy.ts ✅
    • yarn cypress:run --spec cypress/e2e/privileges_integrations_automatic_import.cy.ts ✅
    • yarn cypress:run --spec cypress/e2e/integrations_automatic_import.cy.ts
  • manual testing: create and install an integration via the automatic-import plugin ✅
  • unit tests:
    • node scripts/jest -u x-pack/solutions/security/plugins/security_solution_serverless ✅
    • node scripts/jest -u x-pack/platform/plugins/shared/automatic_import ✅

Follow-up

Once the PR is merged, the following dashboards will need to be updated to reflect the Telemetry keys changes:
Looker: https://lookerstudio.google.com/u/0/reporting/5eb00475-46bc-4c57-93da-5b2d00058c8a/page/p_t7j5zagckd?pli=1
Stack Telemetry: https://stack-telemetry.elastic.dev/s/securitysolution/app/dashboards#/view/e7662c09-eedb-49aa-9615-0600fcb[…]60000),time:(from:now-30d%2Fd,to:now))

@haetamoudi haetamoudi self-assigned this Jan 21, 2025
Copy link
Contributor

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

@haetamoudi
Copy link
Contributor Author

buildkite test this

@haetamoudi
Copy link
Contributor Author

The api docs offenses were already present with the old plugin's name. I have created a follow-up issue to fix them #208113

@haetamoudi haetamoudi force-pushed the 197621-rename-plugin-to-automatic_import branch from deb8565 to edaa558 Compare January 23, 2025 19:27
@haetamoudi haetamoudi marked this pull request as ready for review January 23, 2025 21:50
@haetamoudi haetamoudi requested review from a team as code owners January 23, 2025 21:50
@haetamoudi haetamoudi added backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes labels Jan 23, 2025
@haetamoudi haetamoudi changed the title 197621 rename plugin to automatic import [Automatic Import] rename plugin to automatic import Jan 23, 2025
@jbudz
Copy link
Member

jbudz commented Jan 24, 2025

Is this strictly a rename? Do we know why there's +1.1mb on async bundles?

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

Fleet changes LGTM 🚀

@haetamoudi
Copy link
Contributor Author

Is this strictly a rename? Do we know why there's +1.1mb on async bundles?

@jbudz
I understand it considers it as a completely new plugin instead of an update of the existing one

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

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

x-pack/test/tsconfig.json lgtm

Copy link
Contributor

@bhapas bhapas left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -5,6 +5,7 @@ pageLoadAssetSize:
aiops: 32733
alerting: 106936
apm: 64385
automaticImport: 25433
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this different from the original one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated it by running the command node scripts/build_kibana_platform_plugins.js --update-limits. Is there another process to update this file?

@ilyannn
Copy link
Contributor

ilyannn commented Jan 24, 2025

FYI the build fails because of api_docs/integration_assistant.mdx which should probably be deleted as api_docs/automatic_import.mdx already exists.

@ilyannn
Copy link
Contributor

ilyannn commented Jan 24, 2025

I suggest we add a release note about the rename.

@haetamoudi haetamoudi force-pushed the 197621-rename-plugin-to-automatic_import branch from d3b0817 to 46889e6 Compare January 24, 2025 13:02
* under the `allowedExperimentalValues` object
*
* @example
* xpack.integration_assistant.enableExperimental:
* xpack.automatic_import.enableExperimental:
Copy link
Contributor

Choose a reason for hiding this comment

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

How confident are we that no customers have xpack.integration_assistant set? If any cluster has a xpack.integration_assistant.* config option set then Kibana would fail to start once they upgrade.

If that's a possibility we might need to keep an xpack.integration_assistant plugin which has only a config deprecation or look at other ways to avoid an upgrade failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we had

xpack.integration_assistant.enabled: true xpack.integration_assistant.enableExperimental: ['generateCel']

Of which, xpack.integration_assistant.enabled is default enabled on all clusters.. And xpack.integration_assistant.enableExperimental is introduced as internal only.

But as you mentioned it might be better to keep an xpack.integration_assistant plugin which has only a config deprecation

Copy link
Contributor

Choose a reason for hiding this comment

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

If I think more , this config xpack.integration_assistant.enabled is disabled for few customers, which means this should be deprecated in all 8.x deployments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot we had the ability to do this without keeping the old plugin by using renameFromRoot

https://github.com/elastic/kibana/blob/main/docs/developer/architecture/core/configuration-service.asciidoc?plain=1#L135-L149

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added renameFromRoot for the 2 config options and tested with having

xpack.integration_assistant.enabled: true
xpack.automatic_import.enabled: true
xpack.automatic_import.enableExperimental: ['generateCel']
xpack.integration_assistant.enableExperimental: ['generateCel']

in kibana.dev.yml. No issues

Copy link
Contributor

Choose a reason for hiding this comment

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

May be figure out a way to test upgrade with existing deployment and upgrade to changes in your PR.

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Changes in test/plugin_functional/test_suites/core_plugins/rendering.ts LGTM

@haetamoudi haetamoudi force-pushed the 197621-rename-plugin-to-automatic_import branch from e5d9f99 to c5aeadc Compare January 27, 2025 15:15
@haetamoudi haetamoudi added the Team:Security-Scalability Team label for Security Integrations Scalability Team label Jan 28, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-scalability (Team:Security-Scalability)

@haetamoudi haetamoudi added Feature:AutomaticImport release_note:deprecation and removed release_note:skip Skip the PR/issue when compiling release notes labels Jan 28, 2025
…:haetamoudi/kibana into 197621-rename-plugin-to-automatic_import
Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

I have to say that the name of the new automaticImport plugin is rather vague. I miss the integration noun somewhere, this name doesn't say anything about what it is that is automatically imported. It's too generic IMO.
But anyway, code LGTM.

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

Core changes lgtm and the config deprecation should ensure we don't block upgrades

@elasticmachine
Copy link
Contributor

elasticmachine commented Jan 28, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #8 / Case Owner Selection renders all options
  • [job] [logs] Jest Tests #8 / Connector renders loading state correctly
  • [job] [logs] Jest Tests #8 / CustomFieldsForm renders text as default custom field type
  • [job] [logs] Jest Tests #8 / TableSearch renders with initial value correctly

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
automaticImport - 711 +711

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
automaticImport - 62 +62

Async chunks

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

id before after diff
automaticImport - 1.1MB ⚠️ +1.1MB
fleet 1.7MB 1.7MB -15.0B
securitySolution 21.3MB 21.3MB -50.0B
securitySolutionServerless 126.4KB 126.3KB -26.0B
total ⚠️ +1.1MB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
automaticImport - 4 +4

Page load bundle

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

id before after diff
automaticImport - 10.1KB +10.1KB
securitySolutionServerless 26.5KB 26.5KB -46.0B
total +10.1KB
Unknown metric groups

API count

id before after diff
automaticImport - 78 +78

async chunk count

id before after diff
automaticImport - 2 +2

ESLint disabled in files

id before after diff
automaticImport - 6 +6

ESLint disabled line counts

id before after diff
automaticImport - 7 +7

miscellaneous assets size

id before after diff
automaticImport - 1.2MB ⚠️ +1.2MB

Total ESLint disabled count

id before after diff
automaticImport - 13 +13

History

cc @haetamoudi

@haetamoudi haetamoudi merged commit 06d1661 into elastic:main Jan 28, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:AutomaticImport release_note:deprecation Team:Security-Scalability Team label for Security Integrations Scalability Team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.