sous-chefs / consul

Development repository for the consul cookbook
https://supermarket.chef.io/cookbooks/consul
Apache License 2.0
192 stars 244 forks source link

[Major] Rewrite resources to get rid of poise #598

Closed josqu4red closed 2 years ago

josqu4red commented 3 years ago

Description

Port resources to use Chef native framework, and drop deprecated poise dependencies. Specs are not ported since none were added for newest resources (policy/role/etc).

This is a work in progress, it has been started under criteo-forks/consul-cookbook (https://github.com/criteo-forks/consul-cookbook/pull/19), which is not up-to-date with sous-chefs. Most rewritten resources are currently in use in our environment (not newest ones such as policy/role/token). Some resources (git & package install) are not ported, which we chose not to keep in the fork. Feedback and/or help is very welcomed :)

Issues Resolved

Fixes #551 #555 #590

Check List

kitchen-porter commented 3 years ago
2 Warnings
:warning: This is a big Pull Request.
:warning: This is a Table Flip.

Generated by :no_entry_sign: Danger

ramereth commented 3 years ago

@josqu4red thanks for the PR! Could you please address the cookstyle issues first?

josqu4red commented 3 years ago

@ramereth addressed your comment, and ported remaining policy, role, token resources. Tests are green but coverage is quite low :(

ramereth commented 2 years ago

@ramereth addressed your comment, and ported remaining policy, role, token resources. Tests are green but coverage is quite low :(

It looks like we have a decent amount of integration tests at least so that's OK. We're more worried about that.

ramereth commented 2 years ago

@josqu4red looks like this still depends on the nssm cookbook which also depends on the windows cookbook thus causing the ci to fail due to the lack of unified_mode support in the windows cookbook.

I see that the nssm cookbook is a criteo cookbook. Is there anyway you have access to update that cookbook remove use of the windows cookbook?

josqu4red commented 2 years ago

@ramereth Yes I saw that, we will take care of this soon.

n8dagr87 commented 2 years ago

@ramereth Looks like nssm PR #43 was merged to drop windows dependency - is this ready to rerun checks and merge (assuming they pass)?

ramereth commented 2 years ago

@n8dagr87 unfortunately it looks like the nssm_install needs to have unified_mode true set for this to move forward.

josqu4red commented 2 years ago

Fix incoming https://github.com/criteo-cookbooks/nssm/pull/45

josqu4red commented 2 years ago

Compilation fails with Chef 17.1.35 upward but not with 17.0.242. Suggestions welcome :/

ramereth commented 2 years ago

@josqu4red You need to change the following:

diff --git a/recipes/default.rb b/recipes/default.rb
index 4116277..e58279d 100644
--- a/recipes/default.rb
+++ b/recipes/default.rb
@@ -23,13 +23,13 @@ end

 service_name = node['consul']['service_name']
 config = consul_config service_name do |r|
-  node['consul']['config'].each_pair { |k, v| r.send(k, v) }
+  node['consul']['config'].each_pair { |k, v| send(k.to_sym, v) }
   notifies :reload, "consul_service[#{service_name}]", :delayed
 end

 install = consul_installation node['consul']['version'] do |r|
   if node['consul']['installation']
-    node['consul']['installation'].each_pair { |k, v| r.send(k, v) }
+    node['consul']['installation'].each_pair { |k, v| send(k.to_sym, v) }
   end
 end

@@ -44,6 +44,6 @@ consul_service service_name do |r|
     group node['consul']['service_group']
   end
   if node['consul']['service']
-    node['consul']['service'].each_pair { |k, v| r.send(k, v) }
+    node['consul']['service'].each_pair { |k, v| send(k.to_sym, v) }
   end
 end
josqu4red commented 2 years ago

@ramereth nice, thanks. Any hint on what changed ? I don't see this mentionned in Chef changelog.

ramereth commented 2 years ago

@josqu4red we're in the process of removing circleci/danger from repos. Can you please go ahead and remove Dangerfile and .circleci/config.yml in this PR? That should remove the remaining CI errors.

I'm not sure why that changed but I noticed the r variable was empty when I was debugging. So maybe something changed internally that wasn't meant to be a public API.

kitchen-porter commented 2 years ago

Released as: 5.0.0