-
Notifications
You must be signed in to change notification settings - Fork 123
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
Pass Docker daemon connection params from inventory to connection plugin #157
Pass Docker daemon connection params from inventory to connection plugin #157
Conversation
According to @ytimenkov in #155 (comment), the PR solves #155. |
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.
This looks pretty straightforward to me. Just one inline question.
- Whether to pass all Docker daemon configuration from the inventory plugin to the connection plugin. | ||
- Only used when I(connection_type=docker-api). | ||
type: bool | ||
default: true |
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.
I definitely agree the default should be true
but it is a change in behavior isn't it? Would that consider it breaking?
Or is the current situation such that it couldn't ever be used effectively anyway (in which case there's no need to wait for major version)?
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.
I've thought a bit about it, and I'm still a bit torn; the original issue is a bug report, and I tend to also see the old behavior as a bug. I decided to also add this option to make it possible to revert to the old behavior, so in case someone really needs the old behavior they don't have to stick to an older collection version.
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.
fwiw I agree with you; this is the logical/desired behavior, the previous behavior is a bug. It's always hard to tell when people have relied on buggy behavior. I'm not knowledgeable enough about this plugin to know how likely it is that the new default would have drastically (negative) different behavior; as an end user I would probably prefer something like this released sooner.
@ytimenkov thanks for testing! |
SUMMARY
Fixes #155.
ISSUE TYPE
COMPONENT NAME
docker_containers inventory plugin