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

Don't filter nodes if logdriver==none #2396

Merged
merged 1 commit into from Oct 5, 2017

Conversation

Projects
None yet
3 participants
@cpuguy83
Contributor

cpuguy83 commented Oct 5, 2017

The "none" driver is a special keyword to disable logging, so don't
filter nodes if the "none" driver is not listed on the nodes, since it's
not really a driver and doesn't exist.

Don't filter nodes if logdriver==none
The "none" driver is a special keyword to disable logging, so don't
filter nodes if the "none" driver is not listed on the nodes, since it's
not really a driver and doesn't exist.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 5, 2017

Codecov Report

Merging #2396 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2396      +/-   ##
==========================================
- Coverage   60.62%   60.61%   -0.02%     
==========================================
  Files         128      128              
  Lines       26309    26309              
==========================================
- Hits        15951    15948       -3     
- Misses       8954     8960       +6     
+ Partials     1404     1401       -3

codecov bot commented Oct 5, 2017

Codecov Report

Merging #2396 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2396      +/-   ##
==========================================
- Coverage   60.62%   60.61%   -0.02%     
==========================================
  Files         128      128              
  Lines       26309    26309              
==========================================
- Hits        15951    15948       -3     
- Misses       8954     8960       +6     
+ Partials     1404     1401       -3
@thaJeztah

LGTM

@@ -169,7 +169,7 @@ func (f *PluginFilter) Check(n *NodeInfo) bool {
}
}
if f.t.Spec.LogDriver != nil {
if f.t.Spec.LogDriver != nil && f.t.Spec.LogDriver.Name != "none" {

This comment has been minimized.

@nishanttotla

nishanttotla Oct 5, 2017

Contributor

@cpuguy83 do we need to check for "None" too or is this a case-sensitive keyword?

@nishanttotla

nishanttotla Oct 5, 2017

Contributor

@cpuguy83 do we need to check for "None" too or is this a case-sensitive keyword?

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 5, 2017

Contributor

We could check lower, but it is case sensitive in docker at least.

@cpuguy83

cpuguy83 Oct 5, 2017

Contributor

We could check lower, but it is case sensitive in docker at least.

This comment has been minimized.

@nishanttotla

nishanttotla Oct 5, 2017

Contributor

Okay, in that case we can leave it as is for now.

@nishanttotla

nishanttotla Oct 5, 2017

Contributor

Okay, in that case we can leave it as is for now.

This comment has been minimized.

@thaJeztah

thaJeztah Oct 5, 2017

Member

Also wondered if nil and "none" are the same, but I think they have a different meaning (nil being: use the default?) If not, and the are equal then ideally we should change "none" to nil in the spec

@thaJeztah

thaJeztah Oct 5, 2017

Member

Also wondered if nil and "none" are the same, but I think they have a different meaning (nil being: use the default?) If not, and the are equal then ideally we should change "none" to nil in the spec

@nishanttotla

LGTM

(just one comment)

@nishanttotla nishanttotla merged commit a824e6c into docker:master Oct 5, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 60.61% (target 0%)
Details
dco-signed All commits are signed
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 5, 2017

Contributor
Contributor

cpuguy83 commented Oct 5, 2017

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