[Plugins Discovery] Enforce camelCase plugin IDs #90752
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
afharo
added
Feature:Plugins
Team:Core
afharo
force-pushed
the
plugins-discovery/enforce-camelCase-ids
branch
2 times, most recently
from
d538022 to
f4d3afa
Compare
afharo
force-pushed
the
plugins-discovery/enforce-camelCase-ids
branch
from
f4d3afa to
fcac06d
Compare
⏳ Build in-progress, with failures
Failed CI Steps
History
To update your PR or re-run it, just comment with: |
|
@elasticmachine merge upstream |
|
Pinging @elastic/kibana-core (Team:Core) |
azasypkin
approved these changes
Feb 10, 2021
YulNaumenko
approved these changes
Feb 10, 2021
michaelolo24
approved these changes
Feb 10, 2021
Bamieh
approved these changes
Feb 10, 2021
TinaHeiligers
approved these changes
Feb 10, 2021
There was a problem hiding this comment.
nit: We need to update the examples in the docs we give for migrating legacy plugins to the Kibana Platform. The example uses an id that's not camel case but it's not snake case either, however, if the example plugin id was changed to demoPlugin from demoplugin, it gives a great starting point for folks who tend to copy-paste.
Code LGTM.
| if (!packageInfo.dist && !isCamelCase(manifest.id)) { | ||
| log.warn(`Expect plugin "id" in camelCase, but found: ${manifest.id}`); | ||
| if (!isCamelCase(manifest.id)) { | ||
| throw PluginDiscoveryError.invalidManifest( |
There was a problem hiding this comment.
This PR doesn't have an associated issue with it and it's a little difficult to determine why we're enforcing camel case now when we didn't originally. If we're wrapping up the platform migration work (we had to be more lenient while plugins were still migrating over), then changing from a log warning to an explicit error makes sense.
There was a problem hiding this comment.
You are right! And I couldn't find an issue stating the reasoning behind this. The only thing I could find is the documentation about the Manifest file. It claims "Identifier of the plugin. Must be a string in camelCase. Part of a plugin public contract. Other plugins leverage it to access plugin API, navigate to the plugin, etc.".
I also found that the plugin-generator already creates the id as camelCased:
Bamieh
requested changes
Feb 10, 2021
|
Thank you all for your reviews! 🧡 @TinaHeiligers I'll update the docs straight-away! @Bamieh I did raise that question internally about considering these breaking changes, the answer was "plugins have never been declared as a public API. they aren’t subject to semver". Maybe we can flag them as breaking changes in the release notes but still ship it in a minor because the workaround requires very minimal changes? What do you think? cc @restrry UPDATE: The plugin-generator already creates the IDs as camelCased, and the documentation about the Manifest file claims it must be camelCased. Ideally, this shouldn't generate too much of an issue to third-party plugins if they used the tool and/or read the docs. |
Yea, plugin API is not even considered experimental or beta yet (even if we really should consider doing that in 8.0 imho) And I even think it's better to do that 'breaking' change sooner than later, as the longer we wait, the more 3rd plugins we will break with that change.
I think flagging this as |
afharo
added
release_note:plugin_api_changes
Bamieh
approved these changes
Feb 11, 2021
|
I've created #91124 to handle the doc changes on a separate issue. It looks to me like updating the docs does not only need us to change the name but also the paths and HTTP routes to be fully compliant with our STYLEGUIDE. |
afharo
added a commit
to afharo/kibana
that referenced
this pull request
Feb 11, 2021
Co-authored-by: Kibana Machine <[email protected]>
afharo
added a commit
that referenced
this pull request
Feb 11, 2021
Co-authored-by: Kibana Machine <[email protected]>
|
@afharo For the script to pick up this PR for the API changes doc, you need to use "Dev Docs" instead of "Release Notes" in the summary. I updated the summary. |
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/cross_cluster_replication/feature_controls/ccr_security·ts.Cross Cluster Replication app feature controls security global all privileges (aka kibana_admin) should not render the "Data" sectionStandard OutStack TraceKibana Pipeline / general / X-Pack Accessibility Tests.x-pack/test/accessibility/apps/search_profiler·ts.Accessibility Search Profiler Editor click on the view details linkStandard OutStack TraceKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/modules/setup_module·ts.apis Machine Learning modules module setup sets up module data for logs_ui_categories with prefix, startDatafeed true and estimateModelMemory trueStandard OutStack TraceMetrics [docs]
History
To update your PR or re-run it, just comment with: |
|
@gchaps noted! I'll use it right next time. Sorry for the confusion. |
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
This PR enforces all the plugins' IDs to be camelCase (from warning to actual error) and changes all the test plugins that were not fulfilling this rule.
Checklist
Delete any items that are not applicable to this PR.
For maintainers
Dev Docs
Plugins' IDs in the
kibana.jsonmanifest must be camelCased. It has always been a requirement in the Kibana Platform but, up until now, Kibana only logged the deprecation warning. Now it refuses to start.