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

Remove duplicated "data_dir" attribute #269

Merged
merged 1 commit into from
Feb 2, 2016

Conversation

legal90
Copy link
Contributor

@legal90 legal90 commented Jan 29, 2016

I guess it was just a typo in #259
We already have ['config']['data_dir'], so ['service']['data_dir'] is not needed.

Otherwise it could cause a Consul agent start failure if you override only the one of these attributes.

cc: @Ginja

It should be ['config']['data_dir'] instead of ['service']['data_dir']
@codecov-io
Copy link

Current coverage is 61.90%

Merging #269 into master will increase coverage by +13.23% as of f1abf0c

@@            master   #269   diff @@
=====================================
  Files            7      7       
  Stmts          378    378       
  Branches         0      0       
  Methods          0      0       
=====================================
+ Hit            184    234    +50
  Partial          0      0       
+ Missed         194    144    -50

Review entire Coverage Diff as of f1abf0c

Powered by Codecov. Updated on successful CI builds.

@Ginja
Copy link
Contributor

Ginja commented Jan 29, 2016

Good catch! I override both of these values in our own environment. I'll change this in #263 in case @johnbellone wants to keep ['consul']['service']['data_dir'].

@legal90
Copy link
Contributor Author

legal90 commented Jan 29, 2016

@Ginja Could you please explain what is the original purpose of ['consul']['service']['data_dir'] ?
Why don't you use ['consul']['config']['data_dir'] for that?

@Ginja
Copy link
Contributor

Ginja commented Jan 29, 2016

I can't unfortunately, I was simply modifying what was already there. I don't see a need for it, and am happy to eject it in my PR as well if @johnbellone would like.

@legal90
Copy link
Contributor Author

legal90 commented Jan 29, 2016

Well... I just thought that it was your idea, because this attribute has been added in your commit:
https://github.com/visioncritical/consul-cookbook/commit/ad2fcf5f291f0d325dc3f11d8530c7871dc8337a#diff-25e5d4a4446ae12a0d6f1162b6160375R36

@Ginja
Copy link
Contributor

Ginja commented Jan 29, 2016

Ha, wow. I've been working on this too long I don't even remember what I've done. I also need more coffee.

I added this so I could modify this attribute without overriding the whole resource. Makes it easier to use cross-platform:

https://github.com/visioncritical/consul-cookbook/commit/ad2fcf5f291f0d325dc3f11d8530c7871dc8337a#diff-e7d95569a41fab7bae4ab723c764c914L60
https://github.com/johnbellone/consul-cookbook/blob/master/recipes/default.rb#L55

@scalp42
Copy link
Contributor

scalp42 commented Jan 30, 2016

Seeing the same issue here. Changed our data dir location.

@Ginja
Copy link
Contributor

Ginja commented Jan 30, 2016

I've updated #263 to change it back to just data_prefix_path, which is the original value.

johnbellone added a commit that referenced this pull request Feb 2, 2016
Remove duplicated "data_dir" attribute
@johnbellone johnbellone merged commit 8fabbdf into sous-chefs:master Feb 2, 2016
@legal90 legal90 deleted the fix-attr-dupes branch April 18, 2016 12:41
@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.

5 participants