Skip to content

Conversation

lfrancke
Copy link
Member

@lfrancke lfrancke commented Aug 13, 2025

Description

Important

This should only be merged once stackabletech/docker-images#1225 is merged

Document workaround to disable SNI checks.
I took the opportunity to rewrite the entire FAQ entry.

@lfrancke lfrancke enabled auto-merge August 13, 2025 21:21
@lfrancke lfrancke moved this to Development: Waiting for Review in Stackable Engineering Aug 13, 2025
@lfrancke lfrancke self-assigned this Aug 13, 2025
@lfrancke lfrancke disabled auto-merge August 13, 2025 21:26
sbernauer
sbernauer previously approved these changes Aug 14, 2025
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

LGTM, but we should hold of merging until stackabletech/docker-images#1225 is merged.

Can you please also add a docs: prefix to the PR title? :)
If you have the time I would also be happy about a changelog entry that you can now disable the check via configOverrides (yeah, this is a docker-image thing, but users are not going to read docker-images changelog)

@lfrancke lfrancke changed the title Document workaround to disable SNI checks docs: Document workaround to disable SNI checks Aug 14, 2025
@lfrancke
Copy link
Member Author

I thought about that and wasn't sure.
We can also consider whether we want to expose this as a CRD flag.....let's talk about it at the daily and decide how to proceed.

sbernauer
sbernauer previously approved these changes Aug 14, 2025
@sbernauer sbernauer moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Aug 14, 2025
@sbernauer sbernauer assigned NickLarsenNZ and xeniape and unassigned lfrancke Aug 18, 2025
sbernauer
sbernauer previously approved these changes Aug 18, 2025
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Still LGTM, thanks @xeniape for the suggestions!

Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Left some comments and suggestions.

Hopefully we can remove the SNI workarounds soon.

@NickLarsenNZ
Copy link
Member

Not sure who is supposed to resolve or reply to my comments.

@lfrancke
Copy link
Member Author

I hope I caught all comments. It should be ready for review again.

@lfrancke
Copy link
Member Author

lfrancke commented Oct 2, 2025

I have decided to go with my original proposal for the two comments you disagree with, incorporated another and believe you're wrong about the last.
But if things go the way they usually go I end up being the one that was wrong after all. Let's see :)

@lfrancke lfrancke requested a review from xeniape October 6, 2025 11:29
NickLarsenNZ
NickLarsenNZ previously approved these changes Oct 8, 2025
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Alright, ship it :D

@lfrancke
Copy link
Member Author

lfrancke commented Oct 8, 2025

Now it has conflicts and I can't merge it from the phone :) I'll try to fix that in the next few days.

@NickLarsenNZ

This comment was marked as outdated.

@NickLarsenNZ

This comment was marked as outdated.

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

Successfully merging this pull request may close these issues.

4 participants