Add wildcard/multiple server support to nginx_conf resource #2141

jerryaldrichiii · 2017-09-13T03:28:25Z

Fixes #2035
Fixes #1998

Signed-off-by: Jerry Aldrich <[email protected]>

jerryaldrichiii · 2017-09-13T03:36:04Z

lib/resources/nginx_conf.rb

            .map { |path| parse_nginx(path) }
-            .map { |e| data.merge!(e) }
+            .map { |e| data.merge!(e) { |_, v1, v2| v1 + v2 } }


I feel like this is wrong...just not sure how to make it better. When loading server configs from other files (e.g. /etc/nginx/conf.d/*.conf) the merge! would clobber the server value and replace it. This appends the list to the key...but that assumes that v1 and v2 will always be lists...I don't like that assumption.

should probably be an each; also let's extract this piece as it feels to cover some complexity (for deep merging)

By "lists" to you mean Arrays? Why wouldn't they be? If that's a concern, how do we protect against it?

We own the entire codepath here, so if there's additional promises that need to be made, we can make them.

adamleff

This looks like a great first start.

I left a note about adding additional test coverage by having it find multiple files. We should make sure when it counters multiple files that it properly iterates of each.

We also need to expand the test coverage to ensure that data from each file is present. So if your sample files include multiple servers, we should have a test that is able to pluck unique data out of each server to ensure we're properly including all data and not accidentally clobbering another server record. That should help raise the confidence level of this change.

adamleff · 2017-09-14T21:33:09Z

test/unit/mock/cmd/find-nginx-confd-example-conf

@@ -0,0 +1 @@
+/etc/nginx/conf.d/example.conf


For completeness of testing, I'd change this to return multiple files to ensure the code actually iterates over each of them.

adamleff · 2017-09-14T21:35:20Z

lib/resources/nginx_conf.rb

            .map { |path| parse_nginx(path) }
-            .map { |e| data.merge!(e) }
+            .map { |e| data.merge!(e) { |_, v1, v2| v1 + v2 } }


By "lists" to you mean Arrays? Why wouldn't they be? If that's a concern, how do we protect against it?

We own the entire codepath here, so if there's additional promises that need to be made, we can make them.

arlimus · 2017-09-14T21:42:53Z

lib/resources/nginx_conf.rb

      end

      # Walk through the remaining hash fields to find more references
      Hash[data.map { |k, v| [k, resolve_references(v, rel_path)] }]
    end
+
+    # Merge conf into data list of parameters.


Needs better docs string 😁

Signed-off-by: Dominik Richter <[email protected]>

arlimus

Agree with Adam's multi-file test case.
Otherwise this looks fantastic, thank you so much!!

@dominik

- Add multiple mocked files in `/etc/nginx/conf.d` - Add multiple server definitions in file selected by wildcard - Modify comments in @dominik's addition Signed-off-by: Jerry Aldrich <[email protected]>

jerryaldrichiii · 2017-09-14T22:16:46Z

@adamleff @arlimus:

Feedback addressed! Let me know what you think.

Signed-off-by: Jerry Aldrich <[email protected]>

arlimus

Thank you for the great updates!!

adamleff · 2017-09-15T00:04:32Z

lib/resources/nginx_conf.rb

+      # Step through all conf items and create combined return value
+      data.merge!(conf) do |_, v1, v2|
+        # If both the data field and the conf field are arrays, then combine them
+        next v1 + v2 if v1.is_a?(Array) && v2.is_a?(Array)


Maybe it's just me, but this use of next feels pretty awkward. Any chance I can convince you guys to make this an if ... elsif ... else instead? I think it would read so much cleaner.

adamleff · 2017-09-15T00:08:34Z

I'd really love to see some tests actually check the data we've parsed, not just the count of objects we expect. It can be done in a later PR though.

- Make tests more descriptive by checking parsed content - Refactor `next` logic to use if/elsif/else Signed-off-by: Jerry Aldrich <[email protected]>

rx294 · 2017-09-15T18:32:29Z

lib/resources/nginx_conf.rb

@@ -93,13 +96,37 @@ def resolve_references(data, rel_path)
      if data.key?('include')
        data.delete('include').flatten
            .map { |x| File.expand_path(x, rel_path) }
+            .map { |x| find_files(x) }.flatten


Thank you for the fix Jerry.
However this solution does not always include files (included with '*') because of issue#2135 (find_files uses the command resource).
I just tested this code over an ssh connection and it does not parse all the included conf files;
It works fine when I tested by changing the find_files code to use bash resource instead of command resource.
Hope you can find a solution for it.

Thank you again Jerry,
Rony

I know we have a logged issue about the command resource specifically, but we should log an issue for the find_files method too so we don't lose that context, and link it to this PR. I'm not going to hold off merging this PR since it's still a step in the right direction and will work fine for users not executing via sudo.

https://giphy.com/gifs/reaction-LSmULmByAQHQs/fullscreen

Haha sorry @jerryaldrichiii .
Thank you @adamleff I have posted and issue for find_files. issue #2157

Add wildcard/multiple server support to nginx_conf

bf6ee73

Signed-off-by: Jerry Aldrich <[email protected]>

jerryaldrichiii requested review from chris-rock, arlimus and adamleff September 13, 2017 03:28

jerryaldrichiii requested a review from a team as a code owner September 13, 2017 03:28

jerryaldrichiii added the in progress label Sep 13, 2017

jerryaldrichiii commented Sep 13, 2017

View reviewed changes

adamleff suggested changes Sep 14, 2017

View reviewed changes

arlimus reviewed Sep 14, 2017

View reviewed changes

separate the merge function for maps in nginx_conf

839bcfe

Signed-off-by: Dominik Richter <[email protected]>

arlimus force-pushed the jerryaldrichiii/add-glob-and-multi-server-support-to-nginx-conf branch from 312d45c to 839bcfe Compare September 14, 2017 21:44

arlimus requested review from a team and removed request for chris-rock September 14, 2017 21:44

arlimus reviewed Sep 14, 2017

View reviewed changes

Address PR feedback

114b26e

- Add multiple mocked files in `/etc/nginx/conf.d` - Add multiple server definitions in file selected by wildcard - Modify comments in @dominik's addition Signed-off-by: Jerry Aldrich <[email protected]>

jerryaldrichiii force-pushed the jerryaldrichiii/add-glob-and-multi-server-support-to-nginx-conf branch from f7474b2 to 114b26e Compare September 14, 2017 22:15

Add documentation fields

f8a9f14

Signed-off-by: Jerry Aldrich <[email protected]>

arlimus approved these changes Sep 14, 2017

View reviewed changes

adamleff reviewed Sep 15, 2017

View reviewed changes

jerryaldrichiii force-pushed the jerryaldrichiii/add-glob-and-multi-server-support-to-nginx-conf branch from 905af36 to b074bc4 Compare September 15, 2017 17:24

Address more feedback

e055bcc

- Make tests more descriptive by checking parsed content - Refactor `next` logic to use if/elsif/else Signed-off-by: Jerry Aldrich <[email protected]>

jerryaldrichiii force-pushed the jerryaldrichiii/add-glob-and-multi-server-support-to-nginx-conf branch from b074bc4 to e055bcc Compare September 15, 2017 18:24

rx294 reviewed Sep 15, 2017

View reviewed changes

adamleff merged commit 9773e1c into master Sep 15, 2017

adamleff removed the in progress label Sep 15, 2017

adamleff deleted the jerryaldrichiii/add-glob-and-multi-server-support-to-nginx-conf branch September 15, 2017 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wildcard/multiple server support to nginx_conf resource #2141

Add wildcard/multiple server support to nginx_conf resource #2141

jerryaldrichiii commented Sep 13, 2017 •

edited by adamleff

Loading

jerryaldrichiii Sep 13, 2017

arlimus Sep 14, 2017

adamleff Sep 14, 2017

adamleff left a comment

adamleff Sep 14, 2017

adamleff Sep 14, 2017

arlimus Sep 14, 2017

arlimus left a comment

jerryaldrichiii commented Sep 14, 2017

arlimus left a comment

adamleff Sep 15, 2017

adamleff commented Sep 15, 2017

rx294 Sep 15, 2017

adamleff Sep 15, 2017

jerryaldrichiii Sep 15, 2017

rx294 Sep 15, 2017

		@@ -0,0 +1 @@
		/etc/nginx/conf.d/example.conf

Add wildcard/multiple server support to nginx_conf resource #2141

Add wildcard/multiple server support to nginx_conf resource #2141

Conversation

jerryaldrichiii commented Sep 13, 2017 • edited by adamleff Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment

jerryaldrichiii commented Sep 14, 2017

arlimus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryaldrichiii commented Sep 13, 2017 •

edited by adamleff

Loading