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

docker.containers: Ensure .labels returns an Array #3673

Merged
merged 4 commits into from Dec 20, 2018

Conversation

Projects
None yet
3 participants
@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii commented Dec 18, 2018

This modifies labels in FilterTable to split values on ,.

Without this the value for labels would be 'foo=bar,spam=eggs'. Which would make docker.containers.where { labels.include?('foo=bar') } not behave as expected.

Many thanks to @amitsaha for helping me solve this!

This fixes #3653

Fix `labels` on Docker containers
This modifies `labels` in FilterTable to split values on `,`.

Without this the value for `labels` would be 'foo=bar,spam=eggs'. Which
would make `docker.containers.where { labels.include?('foo=bar') }`
not behave as expected.

Many thanks to @amitsaha for helping me solve this!

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

@jerryaldrichiii jerryaldrichiii force-pushed the ja/fix-docker-labels branch from fb3381e to 836099a Dec 18, 2018

@jerryaldrichiii

This comment has been minimized.

Copy link
Contributor

jerryaldrichiii commented Dec 18, 2018

Could use someone with more FilterTable knowledge to let me know if this is "correct" holistically.

This allows for docker.containers.where { labels.include?('foo=bar') } but doesn't really do much else and changes the value of lables to return an Array of Arrays instead of an Array of Strings.

A user could also have done docker.containers.where { labels.match?(/foo=bar/) }.

@miah

miah approved these changes Dec 18, 2018

Copy link
Contributor

miah left a comment

Thanks @jerryaldrichiii! I think this looks solid. =)

@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Thanks for the turnaround on this, @jerryaldrichiii ! My main concerns are stylistic, getting a better example, and making sure we handle the no-labels case well. Thanks!

Show resolved Hide resolved lib/resources/docker.rb Outdated
Show resolved Hide resolved docs/resources/docker.md.erb Outdated
@@ -203,8 +203,13 @@ def parse_json_command(labels, subcommand)
# does not include a slash since that is not a valid character in a container name
j['names'] = j['names'].split(',').find { |c| !c.include?('/') } if j.key?('names')

# Split labels on ','
# This allows `its('labels') { should include 'foo=bar' }` to work
j['labels'] = j['labels'].split(',') if j.key?('labels')

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Dec 18, 2018

Contributor

I encourage you to think of the Boy Scout Rule. Consider replacing the j, k, and v variables with names that are meaningful to humans and amenable to grepping.

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Dec 18, 2018

Contributor

Can do!

@@ -203,8 +203,13 @@ def parse_json_command(labels, subcommand)
# does not include a slash since that is not a valid character in a container name
j['names'] = j['names'].split(',').find { |c| !c.include?('/') } if j.key?('names')

# Split labels on ','
# This allows `its('labels') { should include 'foo=bar' }` to work
j['labels'] = j['labels'].split(',') if j.key?('labels')

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Dec 18, 2018

Contributor

Not sure if I understand the code flow here, so this may not be a concern:

Is it possible for the JSON data to not contain the key 'labels', as the conditional suggests? In that case, how will your users expect the labels field to behave in the InSpec resource? As-is, I suspect that the `labels column will end up with nil values, so

its('labels') { should include 'something-i-care-about' }

will throw a NoMethod error, looking for #include? on Nil::NilClass.

Consider setting it to be an empty array if there are no labels to parse.

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Dec 18, 2018

Contributor

I just copied what was above to match style. Given the Boy Scout Rule you mentioned, I'll change both to be more Ruby like (e.g. if j['names]). I'll also set labels to an empty array if the field isn't present.

Show resolved Hide resolved test/unit/mock/cmd/docker-ps-a
Respond to feedback
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Looks like there is a failing unit test on the label reader. Other issues are resolved, good work!

See https://travis-ci.org/inspec/inspec/jobs/469749939#L1103

@@ -24,6 +24,7 @@
_(resource.tag).must_equal '12.04'
_(resource.command).must_equal '/bin/bash'
_(resource.ports).must_equal ''
_(resource.labels).must_equal ['app=example', 'version=1.5.4']

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Dec 19, 2018

Contributor

This currently has a failing unit test, which needs to be fixed.

Also, you mentioned that there was an existing unit test which tested a container with zero labels; could you show me where that is?

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Dec 19, 2018

Contributor

Yup, looks like I needed to fix the docker_container resource since labels[0] is no longer correct.

Also, I didn't see where a test where a docker container was explicitly returning [] when no labels are used. Couldn't hurt to have an explicit test instead of relying on docker.containers.labels being correct. I'll make it so.

jerryaldrichiii added some commits Dec 19, 2018

Fix `docker_container` resource
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
Add test for `docker_container` with no labels
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@clintoncwolfe

This comment has been minimized.

Copy link
Contributor

clintoncwolfe commented Dec 20, 2018

Our TravisCI setup currently has an issue, breaking unit and functional tests.

I ran unit and functional tests locally, success, on ref

d72b65df (HEAD -> ja/fix-docker-labels, origin/ja/fix-docker-labels) Add test for `docker_container` with no labels

@clintoncwolfe clintoncwolfe changed the title Fix `labels` on Docker containers docker.containers: Ensure .labels returns an Array Dec 20, 2018

@clintoncwolfe clintoncwolfe merged commit 91fe4ad into master Dec 20, 2018

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
DCO This commit has a DCO Signed-off-by
Details
expeditor/config-validation Validated your Expeditor config file
Details

@clintoncwolfe clintoncwolfe deleted the ja/fix-docker-labels branch Dec 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment