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.

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

Support Docker 23.0 compatibility #2427

Conversation

yankay
Copy link
Contributor

@yankay yankay commented Aug 10, 2023

Support Docker 23.0 compatibility , to fix #2421 .

  1. Support the nerdctl volume prune -a
  2. Change the nerdctl system prune --volume behavior to skip the name volume.
  3. Skip the test InvalidBuildArgCausesError

@fahedouch fahedouch marked this pull request as draft August 10, 2023 09:29
@yankay yankay force-pushed the support-test-integration-docker-23-compatibility branch 2 times, most recently from 75bfa8a to b4cd36f Compare August 10, 2023 10:55
t.Run("InvalidBuildArgCausesError", func(t *testing.T) {
base.Cmd("build", buildCtx, "-t", imageName, "--build-arg", "=TEST_STRING").AssertFail()
t.Run("InvalidBuildArgNotCausesError", func(t *testing.T) {
base.Cmd("build", buildCtx, "-t", imageName, "--build-arg", "=TEST_STRING").AssertOK()
Copy link
Member

Choose a reason for hiding this comment

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

Seems to be a bug of Docker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set Buildx and BuildKit as the default builder on Linux. moby/moby#43992 from https://docs.docker.com/engine/release-notes/23.0/.

So there are some change of the build-args.

Should the test be removed , because it may be a bug :-)

Copy link
Member

Choose a reason for hiding this comment

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

Reported:

The test can be removed

@yankay yankay force-pushed the support-test-integration-docker-23-compatibility branch 6 times, most recently from 0003448 to 0844a51 Compare August 14, 2023 05:56
@yankay yankay changed the title [WIP]Support Docker 23.0 compatibility Support Docker 23.0 compatibility Aug 14, 2023
@yankay
Copy link
Contributor Author

yankay commented Aug 14, 2023

HI @AkihiroSuda

Would you please review the PR again ? :-)

@yankay yankay marked this pull request as ready for review August 14, 2023 08:10
@AkihiroSuda AkihiroSuda added this to the v1.5.1 (tentative) milestone Aug 14, 2023
@AkihiroSuda AkihiroSuda added the area/ci e.g., CI failure label Aug 14, 2023
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit b18c4a1 into containerd:main Aug 14, 2023
21 checks passed
Signed-off-by: Kay Yan <kay.yan@daocloud.io>
@yankay
Copy link
Contributor Author

yankay commented Aug 15, 2023

Thanks @AkihiroSuda for the PR review :-)

base.Cmd("volume", "create", tID+"-1").AssertOK()
base.Cmd("volume", "create", tID+"-2").AssertOK()

base.Cmd("run", "-v", fmt.Sprintf("%s:/volume", tID+"-1"), "--name", tID, testutil.CommonImage).AssertOK()
defer base.Cmd("rm", "-f", tID).Run()

base.Cmd("volume", "prune", "-f").AssertOutContains(tID + "-2")
base.Cmd("volume", "prune", "-f").AssertOutContains(vID)
base.Cmd("volume", "prune", "-a", "-f").AssertOutContains(tID + "-2")
Copy link
Contributor

Choose a reason for hiding this comment

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

@yankay @AkihiroSuda

Curious is this a breaking change which changes the behaviour of nerdctl volume prune -f without -a? Or any breaking change which is for Docker compliance is not considered as breaking change in Nerdctl?

Docker bumped major version with this change so fine in Docker side. https://docs.docker.com/engine/release-notes/23.0/#bug-fixes-and-enhancements-1

Copy link
Member

Choose a reason for hiding this comment

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

This seems too subtle to bump up the major version for nerdctl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci e.g., CI failure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test-integration-docker-compatibility: support Docker/Moby 23
3 participants