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

consul_config: Remove the default value of "server" param #424

Merged
merged 5 commits into from
Apr 23, 2017

Conversation

vsudilov
Copy link
Contributor

@vsudilov vsudilov commented Apr 7, 2017

closes #423

Wasn't sure how you'd prefer to add tests, since server:false is explicitly set in .kitchen.yml. Happy to iterate on that as needed.

@legal90
Copy link
Contributor

legal90 commented Apr 7, 2017

@vsudilov Thanks for your contribution!
However, since the default value is currently hard-coded in the resource, I think we should fix it there, rather then overriding via node's attribute:

https://github.com/johnbellone/consul-cookbook/blame/7b9b75c86ba4963dce6bebd94f289b258942062a/libraries/consul_config.rb#L92

P.s. It should be enough just to remove default: true statement there.

@codecov-io
Copy link

codecov-io commented Apr 7, 2017

Codecov Report

Merging #424 into master will increase coverage by 4.44%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
+ Coverage   58.38%   62.82%   +4.44%     
==========================================
  Files           7        7              
  Lines         346      347       +1     
==========================================
+ Hits          202      218      +16     
+ Misses        144      129      -15
Impacted Files Coverage Δ
libraries/consul_config.rb 97.52% <100%> (+20.02%) ⬆️
libraries/consul_execute.rb 55% <0%> (-45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b9b75c...788641a. Read the comment docs.

@vsudilov
Copy link
Contributor Author

vsudilov commented Apr 7, 2017

Yep, agree that's a much better solution -- commit added.

legal90 added 3 commits April 9, 2017 21:25
We need that to avoid undesired "none" values rendered to the Consul config.
1) "server" is false by default now
2) "bootstrap_expect" is needed for server mode only
@legal90
Copy link
Contributor

legal90 commented Apr 9, 2017

NB: The TravisCI run reveals the very important issue which happens after the removal the default value of "server" attribute (resource parameter).

Since we access this parameter here, it becomes defined with the default value nil, so it is rendered to JSON config file as "server": null, which is not valid.
I've fixed that by removing nil values right before rendering the JSON config file.
@vsudilov I've updated this PR by adding commits to your branch. Now Travis build is green

@johnbellone Please, review this PR.

@legal90 legal90 self-assigned this Apr 9, 2017
@vsudilov
Copy link
Contributor Author

vsudilov commented Apr 9, 2017

Thanks :)

@legal90 legal90 modified the milestone: v3.0.0 Apr 11, 2017
@legal90 legal90 merged commit da7f406 into sous-chefs:master Apr 23, 2017
@legal90 legal90 changed the title attributes: explicit node.default server->false consul_config: Remove the default value of "server" param Jun 11, 2017
@lock
Copy link

lock bot commented Apr 25, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
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.

Default recipe starts consul with server: true
3 participants