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

add nginx_conf accessors for http, servers, and locations #2119

Merged
merged 7 commits into from
Sep 6, 2017

Conversation

arlimus
Copy link
Contributor

@arlimus arlimus commented Sep 4, 2017

Resolves #1979

... well `http` entries really, but we couldnt just call it `https`.

the goal is to `nginx_conf.http` / `nginx_conf.servers` / `nginx_conf.locations` and then also have these calls cascaded down to simplify the access to these fields. the current pattern is rather tedious since we need to check for nil everywhere.
@arlimus arlimus requested a review from a team as a code owner September 4, 2017 18:15
Signed-off-by: Dominik Richter <dominik.richter@gmail.com>
Signed-off-by: Dominik Richter <dominik.richter@gmail.com>
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great step in a better direction, @arlimus - thank you!

We need to do some better defense against a user passing in a path that is not readable or doesn't exist lest we try to call methods on nil

Also, I'm curious as to your use of FilterTable in the classes where the table really only has two columns: the key and the contents. While it's elegant for us as developers, it's not elegant for our users that are going to be calling #flatten and #first a lot to get at the actual content (because of the nested arrays). I wonder if there's a better way to handle this.


# nginx_conf

Use the `nginx_conf` InSpec resource to test configuration data for the NginX server located at `/etc/nginx/nginx.conf` on Linux and Unix platforms.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style-wise, the nginx project refers to the program as nginx (all lowercase) or NGINX (all uppercase) but never mixed case. We should probably respect their style and use nginx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch!

def to_s
@parent.to_s + ', http entries'
end
alias inspect to_s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there's a real specific need to do this, I'd rather leave inspect alone as it can prove useful when debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should decide if we want the inspec shell to have the pretty-printed resources or their inspect equivalent. for example: nginx_conf right now in the inspec shell will pretty-print to => nginx_conf /etc/nginx/nginx.conf. without this change it would print the entire page full of param debug. I aimed for consistency and for less printing, especially on http, but i also get the argument for the counterpoint (not to forget fewer changes).

attr_reader :entries
def initialize(params, parent)
@parent = parent
@entries = params.map { |x| NginxConfHttpEntry.new(x, parent) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I initialize nginx_conf with a path to a file that doesn't exist or has no content, params will be nil and you'll try to call #map on NilClass, which will not be fun :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, also happens when the file has non NGINX content! fixed for now by preventing params from being nil


def to_s
# go two levels up: 1. to the http entry and 2. to the root nginx conf
@parent.parent.to_s + ', server entry'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can be even more transparent with users, especially when they're debugging / exploring in InSpec shell. Otherwise you get output like this:

inspec> a.http.servers.entries
=> [nginx_conf /Users/aleff/projects/inspec/test/unit/mock/files/nginx.conf, server entry,
 nginx_conf /Users/aleff/projects/inspec/test/unit/mock/files/nginx.conf, server entry]

Can we do something like this instead?

def to_s
  server_name = ''
  unless params['server_name'].flatten.nil?
    server_name = params['server_name'].flatten.first
    server_name += ":#{params['listen'].flatten} unless params['listen'].flatten.nil?
  end
  @parent.parent.to_s + ", #{server_name} server entry"
end

^^^ not perfectly tested, but hopefully you get the gist of what I'm trying to describe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, totally! I wanted to leave it out of this PR to figure out pretty-printing for all 3 new nginx portions in a follow-up PR. Are you ok if we issue this in a follow-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added both of these to this PR, thank you for the discussion!

@adamleff adamleff added the Type: Enhancement Improves an existing feature label Sep 5, 2017
for location, http, and servers

Signed-off-by: Dominik Richter <dominik.richter@gmail.com>
@adamleff
Copy link
Contributor

adamleff commented Sep 6, 2017

Thanks for the params-nil cleanup :)

as suggested by @adamleff, thank you!

Signed-off-by: Dominik Richter <dominik.richter@gmail.com>
Signed-off-by: Dominik Richter <dominik.richter@gmail.com>
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks @arlimus - let's get this out there.

@adamleff adamleff merged commit 19ab22f into master Sep 6, 2017
@arlimus arlimus deleted the dom/nginx_conf branch September 6, 2017 12:19
adamleff added a commit that referenced this pull request Sep 6, 2017
The change in #2119 necessitates a minor bump.

Signed-off-by: Adam Leff <adam@leff.co>
@adamleff adamleff mentioned this pull request Sep 6, 2017
adamleff added a commit that referenced this pull request Sep 6, 2017
The change in #2119 necessitates a minor bump.

Signed-off-by: Adam Leff <adam@leff.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants