Improve local role handling

bmarini / knife-inspect

Inspect your chef repo as is compares to what is on your chef server

MIT License

55 stars 13 forks source link

Improve local role handling #29

Closed kamaradclimber closed 9 years ago

kamaradclimber commented 10 years ago

allow role filename to be different than role.name
test json and rb role files

This patch mimic the Chef::Role.from_disk method which unable to read in subdir (how sad!)

coveralls commented 10 years ago

Coverage increased (+0.5%) when pulling 268a01f5c5e40f7c1aee609bfcfc61f8f658455c on kamaradclimber:roles into 8ba4f0ec821124a21a22028bdf13d21237602ce8 on bmarini:master.

coveralls commented 10 years ago

Coverage increased (+0.5%) when pulling 95fe2d79087fb2842c27ea4d6a2dd8ccfc2b74aa on kamaradclimber:roles into 8ba4f0ec821124a21a22028bdf13d21237602ce8 on bmarini:master.

coveralls commented 10 years ago

Coverage increased (+0.5%) when pulling 95fe2d79087fb2842c27ea4d6a2dd8ccfc2b74aa on kamaradclimber:roles into 8ba4f0ec821124a21a22028bdf13d21237602ce8 on bmarini:master.

coveralls commented 10 years ago

Coverage increased (+0.43%) when pulling 5bbb50a1f369b72b0e66a462674e8cb615840721 on kamaradclimber:roles into 8ba4f0ec821124a21a22028bdf13d21237602ce8 on bmarini:master.

stephenlauck commented 10 years ago

@kamaradclimber If a role filename is different the the name contained in the role wouldn't this cause problems in terms of tracking what role is in source and which role is on the Chef server? I think this would cause confusion and is not the path of least surprise.

kamaradclimber commented 10 years ago

@stephenlauck chef does not use the role filename, knife-inspect should not add this additional constraint.

For instance we store related roles in subfolders which is their prefix. To avoid repetition filename is only the moving part:

roles:
  elasticsearch:
    dedicated_master.rb (role name is elasticsearch_dedicated_mater)
    data_holder.rb (role name is elasticsearch_data_holder)

stephenlauck commented 10 years ago

This goes against the source code being the source of truth if the role name on the server is different than the file name in source control. I also think in general @kamaradclimber is overloading roles and should think about moving things into separate Organizations on the Chef server instead of sub-dir the roles.

gregkare commented 10 years ago

@stephenlauck We already support loading roles in subdirectories because some people do that (#18), as long as it doesn't make the code more complicated I'm fine with adding this feature.

gregkare commented 10 years ago

My bad, I didn't read correctly. I don't think it makes sense to have files that have a different file name than the roles. You can use subfolders but I think the code should be simplified, not made more complicated.

gregkare commented 10 years ago

So, Chef::Role.from_disk cannot be used here anyway, it's trying to load files with a regexp (https://github.com/opscode/chef/blob/ee7f79f699beaf136d93970734a0ec552aabc11b/lib/chef/role.rb#L239-L240), so you'd get an error about duplication when you have a "load_balancer" and "other_load_balancer" roles.

kamaradclimber commented 10 years ago

@gregkare agreed that Chef::Role.from_disk cannot be used because of the several limitations. However, both chef in general and chef_fs (now part of chef) do not enforce any kind of rule on role file naming. Otherwise having a name property in a role file would be redundant with the filename. If you want, I can try to make the code less complicated and/or use more chef parts

@stephenlauck we are not doing any role overloading here. As I mentioned earlier, it is purely a matter a concision in file tree.

kamaradclimber commented 9 years ago

Closing this PR since I got no feedback for a few months. Feel free to reopen if you are interested in the exchange.