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.

Sign up for GitHub

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

Jump to bottom

Network filter #2301

Merged
merged 1 commit into from Oct 23, 2017
Merged

Network filter #2301

merged 1 commit into from Oct 23, 2017

Conversation

ldez
Copy link
Member

@ldez ldez commented Oct 23, 2017 •
edited

Description

Regression from #2244

Fixes #2297

@ldez ldez requested a review from a team as a code owner October 23, 2017 08:34
@traefiker traefiker added this to the 1.4 milestone Oct 23, 2017
vdemeester
vdemeester suggested changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Shouldn't it be both scope and driver when API has the required version (or above) ?

provider/docker/docker.go Outdated
networkListArgs := filters.NewArgs()
networkListArgs.Add("scope", "swarm")
// https://docs.docker.com/engine/api/v1.29/#tag/Network (Docker 17.06)
if hasRequiredAPIVersion(serverVersion.APIVersion, "1.29", true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading this, it makes me thinks it's only for 1.29, where it's on 1.29 and above right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's >1.29

provider/docker/docker_test.go Outdated
test := test
t.Run(test.desc, func(t *testing.T) {

state := hasRequiredAPIVersion2(test.baseVersion, test.requiredVersion, test.fallbackState)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo ? (hasRequireAPIVersion2 ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

(and other question, it's not exported, should we really test that ? 😛)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, and there is https://github.com/moby/moby/blob/master/api/types/versions/compare.go too 👼

@ldez ldez changed the title fix(docker): Network filter. Network filter Oct 23, 2017
@nmengin
Copy link
Contributor

nmengin commented Oct 23, 2017 •
edited

@vdemeester
The original PR #2244 was created to check scope instead of driver, I guess this PR just fixes problems due to old Docker versions.

nmengin
nmengin approved these changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏 🐳

vdemeester
vdemeester approved these changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐮

juliens
juliens approved these changes Oct 23, 2017
View reviewed changes
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit e3cac7d into traefik:v1.4 Oct 23, 2017
@traefiker traefiker mentioned this pull request Oct 23, 2017
Closed
@mmatur mmatur mentioned this pull request Oct 24, 2017
Closed
@danieladams456
Copy link
Contributor

@ldez Thanks for the fix! When I opened #2275 right when 1.4.0 came out I assumed it was a wontfix since I was on an old docker version, so I closed it prematurely. Then I see it got fixed as a P0 in #2297. I had been using the RC series for the json access logs -> stdout -> cloudwatch, so this lets me get to the GA series.

@ldez ldez deleted the fix/docker-network-filter branch October 25, 2017 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@juliens juliens juliens approved these changes

@nmengin nmengin nmengin approved these changes

Assignees
No one assigned
Labels
area/provider/docker kind/bug/fix a bug fix size/S
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ldez @nmengin @danieladams456 @vdemeester @juliens @traefiker