Add optimize and --no-optimize flags#16302
Conversation
|
Not against this, but it does seem like a breaking change |
src/cli_plugin/install/install.js
Outdated
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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. |
src/cli_plugin/install/index.js
Outdated
There was a problem hiding this comment.
--skip-optimize was mentioned here, but I am OK with --no-optimize as the option.
There was a problem hiding this comment.
++ 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.