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

[19.03] Fix regression when targeting an old docker engine #2536

Closed
wants to merge 3 commits into from
Closed

[19.03] Fix regression when targeting an old docker engine #2536

wants to merge 3 commits into from

Conversation

silvin-lubecki
Copy link
Contributor

Fixes #2533

- What I did
#cf663b5 introduced a regression when the CLI negotiates the API version with an old daemon. This PR reverts it partially and adds regression tests, running all e2e tests against 18.09 daemon.

- Description for the changelog

  • Fix "Docker client version 19.03.09 doesn't automatically downgrade Docker API version correctly"

- A picture of a cute animal (not mandatory but encouraged)
image

@silvin-lubecki
Partially revert cf663b5 as it breaks the version negotiation with an…
7b2fac4 
… older docker engine.

Signed-off-by: Silvin Lubecki <[email protected]>
@silvin-lubecki silvin-lubecki force-pushed the fix-version-old-engine branch 2 times, most recently from dc364a2 to af4a074 Compare May 19, 2020 17:57
@tiborvass
Copy link
Collaborator

tiborvass commented May 19, 2020 •
edited
Loading

7762578 temporarily reverts the fix, to confirm that Jenkins will fail. I'll force push to what it was after that.

@thaJeztah
Copy link
Member

Looks like jenkins didn't run 😞

@thaJeztah
Copy link
Member

oh, it did? Let me check if that was the e2e as well

@thaJeztah
Copy link
Member

I suspect the options must be after make not before (unless it's expecting env cars and not make vars)?

@tiborvass
Copy link
Collaborator

So I updated Jenkinsfile to the new syntax to make things parallel without changing any other logic. It's now failing without the fix (and I don't know why it wasn't before) but it's also failing in stable and that is because test-e2e-non-experimental target hardcodes 18.09 in 1814122 (this PR). I'm not sure what the rationale was behind that. I'd be in favor of just using $(E2E_ENGINE_VERSION) there as well. @silvin-lubecki @thaJeztah WDYT?

@thaJeztah
Copy link
Member

because test-e2e-non-experimental target hardcodes 18.09 in 1814122 (this PR). I'm not sure what the rationale was behind that

Perhaps @silvin-lubecki was testing that? I suggested we should run the e2e tests in a "matrix" (so run them both against "previous stable" (18.09), and current stable (19.03)).

@silvin-lubecki
Add a new Makefile variable to override DockerInDocker engine version…
1ff6c42 
… we use to run e2e tests

Signed-off-by: Silvin Lubecki <[email protected]>
@silvin-lubecki
Copy link
Contributor Author

My bad @tiborvass, of course it makes no sense, it's a mistake 😄 Fixed!

@silvin-lubecki silvin-lubecki force-pushed the fix-version-old-engine branch 2 times, most recently from def06c8 to 3ba57a4 Compare May 20, 2020 12:58
@silvin-lubecki
Run e2e tests with different engine version on Jenkins
375f685 
Rewrite Jenkinsfile to new declarative syntax without parallel as the e2e framework is not tailored for than (container name clash, port clash,...)

Signed-off-by: Tibor Vass <[email protected]>
Signed-off-by: Silvin Lubecki <[email protected]>
thaJeztah
thaJeztah previously approved these changes May 20, 2020
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@silvin-lubecki silvin-lubecki changed the title Fix regression when targeting an old docker engine [19.03] Fix regression when targeting an old docker engine May 20, 2020
@thaJeztah
Copy link
Member

oh! can we open this against master, and then a separate cherry-pick / backport to 19.03?
That way we don't regress in the next release (20.xx)

@thaJeztah thaJeztah dismissed their stale review May 20, 2020 14:06

see above

@thaJeztah
Copy link
Member

PR on master: #2537
Backport to 19.03: #2538

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

@thaJeztah thaJeztah thaJeztah left review comments

Assignees
No one assigned
Labels
status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@silvin-lubecki @tiborvass @thaJeztah