Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

update default num servers and clients #123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iainthegray
Copy link
Contributor

This is a PR to update the default number of consul servers to 5 as per the hashicorp Reference Architecture and to set the default number of clients to 0

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 8, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Code changes make sense.

Quick sanity check: are you aiming for the root example to implement the Reference Architecture recommendations? If so, perhaps the better option is to just remove the consul clients from the root example entirely and only have them in other examples?

@iainthegray
Copy link
Contributor Author

Quick sanity check: are you aiming for the root example to implement the Reference Architecture recommendations?
Yes.
If so, perhaps the better option is to just remove the consul clients from the root example entirely and only have them in other examples?
I am unsure what the consul-clients do in infrastructure terms. Sure you need a consul client on something, but I cant see a usecase where you would just build an instance with a consul client on it. Hence that is why I have set the default consul clients to 0 then if people want one then they can change that variable

@brikis98
Copy link
Collaborator

I am unsure what the consul-clients do in infrastructure terms. Sure you need a consul client on something, but I cant see a usecase where you would just build an instance with a consul client on it. Hence that is why I have set the default consul clients to 0 then if people want one then they can change that variable

The consul-clients accomplish two things:

  1. They show users an example of how to install the consul agent and run it in consul mode. This pattern can be copied to all your apps/services that need to talk to Consul servers.
  2. Since we have automated tests for all of our example code, including the root example, having the consul clients here provides a way to test that the code in this repo works correctly in client mode.

If the goal with the root example is to implement a Consul Ref Arch, then there's no need to have the client code here. Instead, you could remove it from here, and just ensure one of the other examples has it.

@iainthegray
Copy link
Contributor Author

OK so I understand that, but this PR is simply changing default values in the module. The function of a default value is to show a sensible value that most people can leave untouched. Currently the default for the consul clients is 6 which means that everyone except your tests will have to amend it.
Hence I believe it should be set to 0

@brikis98
Copy link
Collaborator

Currently the default for the consul clients is 6 which means that everyone except your tests will have to amend it. Hence I believe it should be set to 0

Any reason you don't want to just remove the code for the clients *entirely( from the root example if the plan is to make it the RA?

Etiene added a commit that referenced this pull request Mar 14, 2019
Remove boto and pip installation from default installation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants