Skip to content
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 SSL Breaking changes for logstash-output-http to breaking changes… #16701

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

robbavey
Copy link
Member

@robbavey robbavey commented Nov 20, 2024

@donoghuc
Copy link
Member

I like the idea here. For all the other plugins we will continue with sections that have the plugin name and the obsolete parameters. Looks like the docs build is red. The only thing I saw in the logs is a warning about use of headers. Not sure if that is the cause. Once we get this base example sorted out we can start making sure the other plugins get a similar treatment :)

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.

Left suggestions and broader guidelines inline.
Let's discuss and iterate. When we agree, let's apply these style guidelines for the other 10 plugins with changes to SSL settings.

docs/static/breaking-changes-90.asciidoc Outdated Show resolved Hide resolved
@robbavey robbavey marked this pull request as ready for review November 21, 2024 20:58
@robbavey
Copy link
Member Author

@karenzone
Copy link
Contributor

Follow up:

Now that I've played with it, I still like the collapsible content, but I don't like the interaction details in my suggestion.
The down arrow and the link to plugin docs are too close together, and make for an ambiguous information experience. Restating the obvious, if you click the down arrow, the section expands to show deleted settings. If you click the plugin text, you jump to the plugin docs. I'll tweak my suggestion accordingly.

Also, the dropdown arrow is a common UI element and we use it extensively in our docs. I'll put in a note to explain, and we can use it--or not.

Screenshot 2024-11-22 at 3 28 22 PM


We've removed deprecated SSL settings in some {ls} plugins, and have replaced them with updated settings.
If your plugin configuration contains any of these obsolete options, the plugin may fail to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Click the arrow beside a plugin name to see the list of settings that have been
removed and their replacements.

Copy link
Contributor

@karenzone karenzone Nov 22, 2024

Choose a reason for hiding this comment

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

The expandable content is a common UI element, and we use it extensively throughout our docs. If you think we need to call it out here's a suggestion for how and where to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'm good with skipping it. If there is plenty of prior art of us using the control, and not having any explanatory text, then I'm good


[discrete]
[[output-http-ssl-9.0]]
.<<plugins-outputs-http,logstash-output-http>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.<<plugins-outputs-http,logstash-output-http>>
.`logstash-output-http`

Copy link
Contributor

Choose a reason for hiding this comment

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

The link to the plugin docs added confusion and seems like overkill. All of the links in the dropdown content take users to the plugin docs anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants