-
Notifications
You must be signed in to change notification settings - Fork 682
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
new docker_service resource to inspect Docker Swarm services #2456
Conversation
This change adds the `docker_service` resource for Docker swarm mode services. This branches off some of the common elements (id, exists) into a `DockerObject` module along with a utility function for parsing the image/repo string. That function was implemented separately by `docker_image` and `docker_container`, now with a third resource, it made sense to consolidate that into an included module. I used the most comprehensive implementation. Existing classes had to be slightly modified for the genericization. Signed-off-by: Matt Kulka <[email protected]>
Excited for this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattlqx this is a stellar change, very high quality code and documentation contribution. Thank you for that!
I have one question for you in-line before I approve to move this forward. Thank you!
return @info if defined?(@info) | ||
opts = @opts | ||
@info = inspec.docker.services.where { | ||
name == opts[:name] || image == opts[:image] || (!id.nil? && !opts[:id].nil? && (id == opts[:id] || id.start_with?(opts[:id]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why the special treatment around id
? Specifically checking to see if the parameters are not nil
before using them to compare? This is probably worthy of a code comment explaining why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. It's already in master in the docker_container
and docker_image
resources. https://github.com/chef/inspec/blob/master/lib/resources/docker_container.rb#L111
It seems to be saying that if the object has an ID and the resource has an ID, then match if they're equal or if the ID is an abbreviated match (as Docker accepts arbitrarily shorter hash matches for objects if there's only one possibility). I'm not aware of any circumstances where an object in Docker wouldn't have an ID so that check may be over-safe but the rest is pretty legit.
Thanks for the praise. This change is mostly a light refactor of prior art though so most of it should go to @chris-rock . This should make it easier to round out the collection of Docker objects though with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattlqx This is amazing work, huge kudos for the rewrite and improvements as well as the docker_service
resource!!
@mattlqx thanks for this awesomeness |
This change adds the
docker_service
resource for Docker swarm mode services. Thisbranches off some of the common elements (id, exists) into a
DockerObject
module alongwith a utility function for parsing the image/repo string. That function was implemented
separately by
docker_image
anddocker_container
, now with a third resource, it madesense to consolidate that into an included module. I used the most comprehensive
implementation. Existing classes had to be slightly modified for the genericization.
Signed-off-by: Matt Kulka [email protected]