Added --s3.enable-tls flag #148
Conversation
server/config.go
Outdated
| &cli.BoolFlag{ | ||
| Name: S3DisableTLSFlagName, | ||
| Usage: "whether to disable TLS for S3 storage", | ||
| Value: true, | ||
| EnvVars: prefixEnvVars("S3_DISABLE_TLS"), | ||
| }, |
There was a problem hiding this comment.
I personally don't like having a BoolFlag set to true by default, because then actually setting the flag --s3.disable-tls is useless, so the only flag config one would ever use is --s3.disable-tls=false which feels counterintuitive.
I'd personally prefer to flip to --s3.enable-tls, but @epociask wdyt?
There was a problem hiding this comment.
I tried to keep a similar syntax to --eigenda-disable-tls but it's true and logic your point. I'll follow your suggestion, and we can revert or change to something else if @epociask prefers in other way.
jcortejoso
force-pushed
the
jcortejoso/s3-tls-flag
branch
from
24b7c37
to
e44e1a7
Compare
store/precomputed_key/s3/cli.go
Outdated
| Name: EnableTLSFlagName, | ||
| Usage: "enable TLS connection to S3 endpoint", | ||
| Value: false, | ||
| EnvVars: withEnvPrefix(envPrefix, "S3_ENABLE_TLS"), |
There was a problem hiding this comment.
WithEnvPrwrix adds the S3 prefix. Should be removed. Seems like we forgot to remove it on some other flags as well. Do you mind changing those as well?
Changes proposed
Adding a new
--s3.enable-tlscommand line flag to optionally enable SSL connection with the S3 endpoint.Default value for the new flag is
falseto keep current behaviour.I have tested with GCP GCS (
--s3.endpoint=storage.googleapis.com), that requires a secure connection.Screenshots (Optional)
Note to reviewers