-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add optimize and --no-optimize flags #16302
Conversation
Not against this, but it does seem like a breaking change |
@@ -30,8 +30,6 @@ export default async function install(settings, logger) { | |||
|
|||
await renamePlugin(settings.workingPath, path.join(settings.pluginDir, settings.plugins[0].name)); | |||
|
|||
await rebuildCache(settings, logger); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we kept rebuildCache
as the default, but added a skip-optimize
or something like that would skip rebuild? Then users could opt into this. We could also document this as the "recommended approach", but we can't make it a default just yet because it would be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on taking a separate approach for 6.x (if any)? I'm fine targeting this for 7.0 only even if it's getting removed.
I ask because we do have workarounds documented, but there's so many bin/plugin install guides that's its usually not followed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the reason I'm so heavy handed in this PR is I'm not sure why we bundle on plugin install. I've had suspicions that we went this way on v1 and never revisited, but if there's more context I'd love to hear it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Targeting this for K7 is okey with me (even if that means making no changes to 6.x).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do --skip-optimize for 6.x and --optimize for 7.0. Just make sure to add it to the breaking changes in master. And we should update all references in our docs to use --skip-optimize to help folks migrate to this pattern sooner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if you do what I just suggested, I strongly recommend making one PR that makes the --skip-optimize change to both master/6.x, then follow up with a PR that adds the --optimize argument, breaking change doc, and a clear error message when someone uses --skip-optimize
@elastic/kibana-operations I was about to close this out with #7322 as the approach but I wanted to throw up a refactor of this for a quick glance. It's lower risk with no breaking changes, and when 7322 is in this would hopefully be a simple revert. |
💚 Build Succeeded |
@jbudz want to make the changes mentioned in #16302 (comment) and we can move forward with this PR? |
I can but that was how it was organized originally. I think the comment implies getting this merged and backported, and then opening a breaking PR to master on top of this one. Either way not a problem, lemme know. |
Updated the top level description to indicate current state, sorry about that. |
@@ -47,6 +47,7 @@ export default function pluginInstall(program) { | |||
.command('install <plugin/url>') | |||
.option('-q, --quiet', 'disable all process messaging except errors') | |||
.option('-s, --silent', 'disable all process messaging') | |||
.option('--no-optimize', 'disable optimization after plugin extraction') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--skip-optimize
was mentioned here, but I am OK with --no-optimize
as the option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ for Commander convention.
@jbudz I pointed out one inconsistency with the feedback. Can we also update the docs as Court mentioned to use this option? |
1302cc2
to
a5843be
Compare
💚 Build Succeeded |
@jbudz this LGTM - just one more small ask. Can we remove Something like: Installing plugins while deferring optimizationThe majority of the time spent installing a plugin is running the optimizer. If you're installing multiple plugins it can make sense to omit that step and only run it once. This can be done by providing |
💚 Build Succeeded |
a5843be
to
1115f63
Compare
💔 Build Failed |
@jbudz looks like this was caught by prettier.
|
5810070
to
4bd12db
Compare
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
I pushed three changes since the approval, tsc/lint/snapshot fixes. Merging away. |
* Add optimize flags * update docs to --no-optimize * docs * extra newline * lint * update mocks * update default config * update snapshots
6.x/5: 0eb4f58 |
This adds an optimize flag to bin/kibana and a no-optimize flag to bin/kibana-plugin. This doesn't change any current behavior beyond adding the new flags. If the optimize flag is used the server will not start after bundling.