Mark previously deprecated SSL settings as obsolete#1197
Mark previously deprecated SSL settings as obsolete#1197donoghuc merged 8 commits intologstash-plugins:mainfrom
Conversation
donoghuc
force-pushed
the
GH-1190-mark-ssl-opts-obsolete
branch
from
2c81627 to
6cc1624
Compare
robbavey
left a comment
robbavey
left a comment
There was a problem hiding this comment.
Code LGTM, over to @karenzone for docs approval
|
Corresponding LS deprecation docs PR elastic/logstash#16787 |
|
I came up with a workflow to run the acceptance tests in LS against these changes un this branch https://github.com/elastic/logstash/tree/test-plugin-from-gh-source-buildkite-run CI runs https://buildkite.com/elastic/logstash-pull-request-pipeline/builds/1958 are red but it appears to be due to installing a gem from GH source instead of ssl deprecation issues. So i think this should not cause any issues once it promotes into LS. |
There was a problem hiding this comment.
DO NOT MERGE: We'll need to review and merge this fix (elastic/logstash#16798) first to avoid breaking the docs build. Then, I'll re-test to be sure we're good to go.
| [id="plugins-{type}s-{plugin}-common-options"] | ||
| include::{include_path}/{type}.asciidoc[] | ||
|
|
||
| :no_codec!: |
There was a problem hiding this comment.
This needs to be at the end of every plugin doc file.
Please add it back. :-)
There was a problem hiding this comment.
OH, wow, yeah my bad! Thanks for catching that.
There was a problem hiding this comment.
Line 328-329: Please change to
"This plugin supports these configuration options plus the <<plugins-{type}s {plugin}-common-options>> described later."
To resolve:
- bad link causing docs build to fail
- inaccurate information (that we're continue to support deleted settings)
There was a problem hiding this comment.
Line 443:
Please change <<plugins-{type}s-{plugin}-ssl,ssl_enabled => true>> to
<<plugins-{type}s-{plugin}-ssl_enabled,ssl_enabled => true>>.
It's looking for the now deleted ssl setting.
(GitHub won't allow me to comment on lines not included in this PR. )
|
@donoghuc, after pulling down your latest update and making that last fix noted Line 443, I'm getting a passing doc build. We're so close! Ugh... now we have some rendering problems. I'll have those for you shortly. |
karenzone
left a comment
karenzone
left a comment
There was a problem hiding this comment.
This change didn't get implemented and is causing the doc build to fail.
Sorry, that got lost in the shuffle. Updated in 7c4be65 |
|
GitHub won't let me make suggestions to lines that didn't have changes. That's the reason, I have to note these as general suggestions with Line numbers. |
| WARNING: As of version `12.0.0` of this plugin, some configuration options have been replaced. | ||
| The plugin will fail to start if it contains any of these obsolete options. | ||
|
|
||
| [cols="<,<",options="header",] |
There was a problem hiding this comment.
Delete extra space before delimiters in table (lines 1333 and 1342)
docs/index.asciidoc
Outdated
|
|
||
| [cols="<,<",options="header",] | ||
| |======================================================================= | ||
| ======================================================================= |
There was a problem hiding this comment.
The pipe in Line 1333 needs to stay, please.
There was a problem hiding this comment.
🤦 i'm really doing poorly today. Need more coffee.
|
|
||
| ## 11.22.10 | ||
| - Add `x-elastic-product-origin` header to Elasticsearch requests [#1194](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/1194) | ||
|
|
There was a problem hiding this comment.
NO CHANGE NEEDED HERE.
ON MERGE, we might have a conflict to resolve. Please be sure to keep what's currently in source.
The correct PR number is #1195.
There was a problem hiding this comment.
I can explicitly rebase against main and push that to this branch. Locally that looks right, i'll push so there is no confusion.
There was a problem hiding this comment.
Pushed the rebase, it looks correct in the expanded diff now :)
- SSL settings that were marked deprecated in version `11.14.0` are now marked
obsolete, and will prevent the plugin from starting.
- These settings are:
- `cacert`, which should be replaced by `ssl_certificate_authorities`
- `keystore`, which should be replaced by `ssl_keystore_path`
- `keystore_password`, which should be replaced by `ssl_keystore_password`
- `ssl`, which should be replaced by `ssl_enabled`
- `ssl_certificate_verification`, which should be replaced by
`ssl_verification_mode`
- `truststore`, which should be replaced by `ssl_truststore_path`
- `truststore_password`, which should be replaced by
`ssl_truststore_password`
Restore the deleted EOF as it is required for docs.
Replace ssl with ssl_enabled.
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
donoghuc
force-pushed
the
GH-1190-mark-ssl-opts-obsolete
branch
from
bd5833f to
41058cb
Compare
karenzone
left a comment
karenzone
left a comment
There was a problem hiding this comment.
Builds cleanly, renders as expected, and LGTM! 🚀
|
Amazing! thanks so much for all the help (and patience) on this one. |
3 participants
11.14.0are now marked obsolete, and will prevent the plugin from starting.cacert, which should be replaced byssl_certificate_authoritieskeystore, which should be replaced byssl_keystore_pathkeystore_password, which should be replaced byssl_keystore_passwordssl, which should be replaced byssl_enabledssl_certificate_verification, which should be replaced byssl_verification_modetruststore, which should be replaced byssl_truststore_pathtruststore_password, which should be replaced byssl_truststore_passwordThanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/
Closes #1190