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 option to enable TLS for minio #62
Conversation
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
), | ||
Readiness: e2e.NewHTTPReadinessProbe("metrics", "/health", 200, 204), | ||
}, | ||
e2e.StartOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor nit here. Why modify the spacing? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mdox, I think we were printing an extra bracket earlier, which caused the syntax to be a bit off. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, thanks! Minor nits, optional 🙂
200, | ||
) | ||
} else { | ||
envVars = append(envVars, "ENABLE_HTTPS="+"0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the default, do we need to specify this if we don't want to enable HTTPS? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll address in follow-up
) | ||
} else { | ||
envVars = append(envVars, "ENABLE_HTTPS="+"0") | ||
command = command + fmt.Sprintf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we could have one command
var instead of two separate in each branch and just (not) include --certs-dir
depending on HTTPS option
* Add option to enable TLS for minio Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> * Fix readiness probe Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> * Fix lint Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
This PR adds an option to enable TLS for minio by generating certs, adjusting commands and readiness probes accordingly.