add deprecation warning for legacy 3rd party plugins #62401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pgayvallet
added
release_note:skip
|
Pinging @elastic/kibana-platform (Team:Platform) |
pgayvallet
commented
Apr 3, 2020
Comment on lines
+136
to
+139
| logLegacyThirdPartyPluginDeprecationWarning({ | ||
| specs: pluginSpecs, | ||
| log: this.log, | ||
| }); |
There was a problem hiding this comment.
Should I also check for disabled plugins (disabledPluginSpecs ) ?
There was a problem hiding this comment.
We want plugin authors to update their code. They don't disable their plugins likely.
Comment on lines
39
to
45
| log.warn( | ||
| `Some installed third party plugin(s) [${pluginIds.join( | ||
| ', ' | ||
| )}] are using the legacy plugin format and will no longer work after version 8.0. ` + | ||
| `Please refer to ${breakingChangesUrl} for a list of breaking changes ` + | ||
| `and ${migrationGuideUrl} for documentation on how to migrate legacy plugins.` | ||
| ); |
There was a problem hiding this comment.
Given that we want to add link to documentation and that cause the message to be quite big, I thought only logging the warning once with all the plugin ids was better.
Input welcome on the actual message. Are these two links enough, or do you see any to add?
There was a problem hiding this comment.
Maybe we should state will no longer work in a future Kibana release in case we decided to remove support earlier than 8.0?
There was a problem hiding this comment.
Ideally we would know which version we plan to drop support at, but realistically I guess this is a reasonable change to protect us. Will do.
Apart from that, do you see anything to change/add in the message?
Comment on lines
+49
to
+52
| const isThirdPartyPluginSpec = (spec: LegacyPluginSpec): boolean => { | ||
| const pluginPath = spec.getPack().getPath(); | ||
| return !internalPaths.some(internalPath => pluginPath.indexOf(internalPath) > -1); | ||
| }; |
There was a problem hiding this comment.
So, this is not the most solid piece of software I ever crafted, but as pack.getPath() returns an absolute url, and as I didn't really want to work with kbnDevUtils.REPO_ROOT and path resolving, I felt this was sufficient, as I think it's very unlikely to have false negatives here.
Tell me if we think this is not sufficient and if this should be improved.
There was a problem hiding this comment.
You could probably get around using kbnDevUtils by simply doing:
const rootPath = Path.resolve(Path.join(__dirname, '..', '..', '..', '..', '..'))Also we may want to make the xpack path more specific x-pack/legacy/plugins so that any plugins loaded in functional tests /x-pack/test/ also get the warning (similar to test/plugin_functional)
There was a problem hiding this comment.
You could probably get around using kbnDevUtils by simply doing
const rootPath = Path.resolve(Path.join(__dirname, '..', '..', '..', '..', '..'))
TBH, I'm not a big fan of relative resolves, as they are imho very fragile to refactoring. If we think this detection implementation is not good enough, I would maybe lean to still use '@kbn/dev-utils' as it will keep the repo root logic in a single place. OTOH the only imports of this module we currently have in src/core/server are in test files. So:
- Is this implementation too fragile and do we want to switch to real absolute path checking? (I'm good with that)
- If we do, should we use
@kbn/dev-utils'sREPO_ROOTor theconst rootPath = Path.resolve(Path.join(__dirname, '..', '..', '..', '..', '..'))alternative?
Also we may want to make the xpack path more specific
x-pack/legacy/plugins
That would be great, except there is technically only ONE xpack plugin/plugin pack, which is x-pack/index.js.
Lines 36 to 42 in 982c0da
We don't have any information on the plugins this pack loads. I can change the xpack check to a regexp check to /x-pack$ (or to strict absolute path checking depending on the answer of my comment's first point) however, to only target the 'real' x-pack plugin and allow to log warnings for functional test plugins.
WDYT?
There was a problem hiding this comment.
- Is this implementation too fragile and do we want to switch to real absolute path checking? (I'm good with that)
- If we do, should we use
@kbn/dev-utils'sREPO_ROOTor theconst rootPath = Path.resolve(Path.join(__dirname, '..', '..', '..', '..', '..'))alternative?
Good point on the brittleness of refactoring. Though it's unlikely we'll refactor or move any of this legacy code in the near future, let's not create this situation if avoidable.
My main reason for not using @kbn/dev-utils is the fact that we don't use that module in any production code except in src/cli.
Given those two points, let's just keep what you have.
We don't have any information on the plugins this pack loads. I can change the xpack check to a regexp check to
/x-pack$(or to strict absolute path checking depending on the answer of my comment's first point) however, to only target the 'real' x-pack plugin and allow to log warnings for functional test plugins.
I forgot about the x-pack mega plugin pattern in legacy. I'm leaning towards just leaving as-is. There is only one plugin in x-pack/test/plugin_functional and it's a KP plugin, so this warning wouldn't apply anyways.
There was a problem hiding this comment.
you can use
const { fromRoot } = require('../../core/server/utils');
fromRoot()to get absolute path to the root
Also we may want to make the xpack path more specific x-pack/legacy/plugins so that any plugins loaded in functional tests /x-pack/test/
Why we cannot reach out their codeowners directly?
…ty-legacy-warnings
…ty-legacy-warnings
joshdover
reviewed
Apr 9, 2020
| const internalPaths = ['/src/legacy/core_plugins', '/x-pack']; | ||
|
|
||
| const breakingChangesUrl = | ||
| 'https://github.com/elastic/kibana/blob/master/docs/migration/migrate_8_0.asciidoc'; |
There was a problem hiding this comment.
Let's use the actual published doc page: https://www.elastic.co/guide/en/kibana/master/breaking-changes-8.0.html
…ty-legacy-warnings
…ty-legacy-warnings
mshustov
approved these changes
Apr 14, 2020
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/security/management·js.security app Management Security navigation Clicking save in create role section brings user back to listingStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
pgayvallet
added a commit
to pgayvallet/kibana
that referenced
this pull request
Apr 17, 2020
* add warnings for legacy 3rd party plugins * use published doc page for 8.0 breaking changes * update message to remove fixed 8.0 version reference * fix generated doc
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Apr 17, 2020
* master: (40 commits) [APM]Upgrade apm-rum agent to latest version to fix full page reload (elastic#63723) add deprecation warning for legacy 3rd party plugins (elastic#62401) Migrate timelion vis (elastic#62819) Replacebad scope link with actual values (elastic#63444) Index pattern management UI -> TypeScript and New Platform Ready (create_index_pattern_wizard) (elastic#63111) [SIEM] Threat hunting enhancements: Filter for/out value, Show top field, Copy to Clipboard, Draggable chart legends (elastic#61207) [Maps] fix term join agg key collision (elastic#63324) [Ingest] Fix agent config key sorting (elastic#63488) [Monitoring] Fixed server response errors (elastic#63181) update elastic charts to 18.3.0 (elastic#63732) Start services (elastic#63720) [APM] Encode spaces when creating ML job (elastic#63683) Uptime 7.7 docs (elastic#62228) [DOCS] Updates remote cluster and ccr docs (elastic#63517) [Maps] Add 3rd party vector tile support (elastic#62084) [Endpoint][EPM] Retrieve Index Pattern from Ingest Manager (elastic#63016) [Endpoint] Host Details Policy Response Panel (elastic#63518) [Uptime] Certificate expiration threshold settings (elastic#63682) Refactor saved object types to use `namespaceType` (elastic#63217) [SIEM][CASE] Create comments sequentially (elastic#63692) ...
pgayvallet
added a commit
that referenced
this pull request
Apr 17, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fix #56558
Logs a deprecation warning explaining that legacy 3rd parties plugins will no longer work post 8.0, and point to associated documentation
Checklist