Skip to content

Conversation

@r-mol
Copy link
Contributor

@r-mol r-mol commented Apr 22, 2023

What was changed

  1. client/factory.go -> Added parsing --tls flag with checking on error. And added the logic that when the flag is activated, the host is taken from the address/localhost
  2. common/defs-flags.go -> Added related definition for --tls flag
  3. common/flags.go -> Added const of --tls flag name. And added --tls flag to SharedFlag slice

Why?

I am connecting to my server over a TLS connection and I had to duplicate my host from the --address flag to the --tls_server_name flag. This cluttered up the command, as well as misleading.

Checklist

  1. Closes #203

  2. How was this tested:

  • By internal tests, but with adding this flag
  • By connecting to local server with this flag
  1. Any docs updates needed? - NO

Related to same issue

temporalio/tctl#360

r-mol added 2 commits April 22, 2023 19:56
* refactor: add new flag --tls, to create tls connection by host from given address

* refactor: rollback spaces

* refactor: rollback spaces

* refactor: change definition of flag

* refactor: change the logic. (If we have server name, then it is not need to get tls flag)

* refactor: rename TLS flag from "FlagEnableTLS" to "FlagTLS"

* refactor: change error message

* refactor: rewrite comment
@CLAassistant
Copy link

CLAassistant commented Apr 22, 2023

CLA assistant check
All committers have signed the CLA.

@bergundy bergundy requested a review from feedmeapples April 25, 2023 00:30
return nil, fmt.Errorf("unable to read TLS disable host verification flag: %w", err)
}
enableTLSS := c.String(common.FlagTLS)
enableTLS, err := strconv.ParseBool(enableTLSS)
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reason to do custom parsing? (instead of c.Bool(..))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm doing custom parsing to keep consistency with your code above:

disableHostNameVerificationS := c.String(common.FlagTLSDisableHostVerification)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok so these values are accepted with strconv.ParseBool, true: "1", "t", "T", "true", "TRUE", "True", false: "0", "f", "F", "false", "FALSE", "False"

I'd go with the c.Bool for consistency with the rest of the CLI

Copy link
Contributor

Choose a reason for hiding this comment

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

updated

@r-mol r-mol requested a review from feedmeapples April 26, 2023 08:52
Co-authored-by: Ruslan <11838981+feedmeapples@users.noreply.github.com>
@feedmeapples
Copy link
Contributor

Tests fail due to recent regression unrelated to the current PR, specifically verifying RC code in TestServerInterruptRC and E2E tests running indefinitely. Approving and fixing the tests in different PR

@feedmeapples feedmeapples merged commit fb903b8 into temporalio:main Apr 29, 2023
@r-mol
Copy link
Contributor Author

r-mol commented Apr 30, 2023

And could you also check the PR with the same issue temporalio/tctl#360.

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.

[Feature Request] Add a TLS configuration based on the URL format

3 participants