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

Make RPC, Serf LAN, Serf WAN port configurable from CLI #4353

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Make RPC, Serf LAN, Serf WAN port configurable from CLI #4353

merged 1 commit into from
Jul 24, 2018

Conversation

azam
Copy link
Contributor

@azam azam commented Jul 6, 2018

Currently RPC, Serf LAN, Serf WAN ports are only configurable through configuration files.
In a deployment environment that chooses command line over configuration files, having to be able to define the ports from the CLI is helpful.

@azam azam changed the title Add serf lan wan port args Make RPC, Serf LAN, Serf WAN port configurable from CLI Jul 6, 2018
@banks
Copy link
Member

banks commented Jul 10, 2018

Hey @azam, thanks for the PR.

We already allow several differnt options for specifying these things so I'd be reluctant to add yet more unless there is a really compelling reason you can't use an existing one.

Is there a reason you can't just use consul agent -hcl 'ports { serf_lan = 1234, serf_wan = 2345 }' etc?

@azam
Copy link
Contributor Author

azam commented Jul 12, 2018

@banks
I happen to work on a build and deploy environment where executables' arguments are scanned and audited, so having a 'normal' argument as opposed to an opaque string argument is preferable for tokenizing and increase visibility to what values goes into which arguments.

Having to use hcl means that we have to parse them (and implement a parser for it) first to achieve what the same level of visibility as with normal arguments.

There are other ways getting around that, and one of them is this change PR. It is a matter of convenience and preference. I got your point, but still I thought this was worth trying.

Feel free to merge or close this.

@banks
Copy link
Member

banks commented Jul 13, 2018

@azam thanks for the context. You present a more compelling reason than any I could come up with so I'll leave this open for discussion on our weekly team call to review community issues and PRs.

Thanks!

@banks banks added the thinking More time is needed to research by the Consul Contributors label Jul 13, 2018
@pearkes
Copy link
Contributor

pearkes commented Jul 13, 2018

Can you add tests for this (flags_test.go)? We agree this is a good addition. See #2263 for some precedent. I don't think dns port is tested from that PR so would be great to add tests for all ports confirmed from the CLI as part of your change.

@pearkes
Copy link
Contributor

pearkes commented Jul 13, 2018

Also if you rebase the CI (Travis) should be a bit more reliable for tests here.

Make RPC port accessible to CLI

Add tests and documentation for server-port, serf-lan-port, serf-wan-port CLI arguments
@azam
Copy link
Contributor Author

azam commented Jul 20, 2018

@banks @pearkes
Sorry for the delay. Thank you for considering this!
Rebased and added tests and doc for the change.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Great thanks for the contribution. Will be in 1.2.2.

@banks banks merged commit e954450 into hashicorp:master Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
thinking More time is needed to research by the Consul Contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants