Skip to content

Conversation

robbavey
Copy link
Contributor

@robbavey robbavey commented Nov 13, 2024

This commit marks SSL settings obsolete that were previously marked as deprecated as part

of the SSL Settings Standardization process implemented in 3.7.0 of this plugin.

Marking these settings as obsolete is a breaking change that will stop the plugin from starting, and forces users to move to the new standard settings.

This commit makes the following configuration settings obsolete:

This commit cleans up some code to handle duplicate settings, and removes tests that were put in place to support the co-existence of deprecated and non-deprecated settings, replacing them with tests that verify that obsolete settings are identified early, and information about the deprecation is related to the user.

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link:

Relates: #181

Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removal looks clean. The only open question/suggestion is consistency on whether we validate an obsolete parameter: #182 (comment)

Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, CI appears to be backed up. Assuming that gets through green this should be g2g :)

@donoghuc
Copy link
Contributor

donoghuc commented Dec 3, 2024

I think test failures are due to expired cert fixtures. Regenerated in #184

Will probably be worth rebasing on that once it lands as this has some overlap.

UPDATE: #184 has been merged. This should be ready for rebase to confirm tests are all passing :)

This commit marks SSL settings `obsolete` that were previously marked as `deprecated` as part of the SSL Settings Standardization process implemented in `3.7.0` of this plugin. Marking these settings as `obsolete` is a *breaking change* that will stop the plugin from starting, and forces users to move to the new standard settings. This commit makes the following configuration settings obsolete: This commit cleans up some code to handle duplicate settings, and removes tests that were put in place to support the co-existence of deprecated and non-deprecated settings, replacing them with tests that verify that obsolete settings are identified early, and information about the deprecation is related to the user.
Copy link
Contributor

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All green after rebase! 🍏

@robbavey robbavey requested a review from karenzone December 10, 2024 21:22
@robbavey
Copy link
Contributor Author

Ready for docs review @karenzone

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@robbavey robbavey merged commit 587efec into logstash-plugins:main Dec 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants