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

exporter: use docker-spec instead of locally defined types #4634

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 10, 2024

The Docker image specification (which extends the OCi types with additional fields) was moved to a separate module, so we can now use those definitions as a central place to define these types.

Comment on lines 6868 to 6870
const expectedDigest = "sha256:29f2980a804038b0f910af98e9ddb18bfa4d5514995ee6bb4343ddf621a4e183"
const expectedDigest = "sha256:3eb3c164e3420bbfcf52c34f1e40ee66631d69445e934175b779551c729f80df"
Copy link
Member Author

Choose a reason for hiding this comment

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

Put this in a separate commit for now, but we need to check if this is expected (this digest is known for having to be updated periodically though, so perhaps it's all ok)

/cc @AkihiroSuda

Copy link
Member

Choose a reason for hiding this comment

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

SGTM, but we may want to know what is causing this change

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, wondering if it's either the strslice type that was used in the local type, or if it's because the new type embeds other types, perhaps changing order of fields 🤔

I didn't look yet if there's an easy way to compare the actual JSON for both (as part of the test perhaps?)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should integrate https://github.com/reproducible-containers/diffoci to the test

Copy link
Member Author

Choose a reason for hiding this comment

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

Would go-cmp already show a proper diff (didn't look yet) assuming we can get the data? Or would that be more complicated?

Copy link
Member

Choose a reason for hiding this comment

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

For a single blob go-cmp could be enough, but it is not so useful for the entire merkle tree

Copy link
Collaborator

Choose a reason for hiding this comment

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

{
  "architecture": "amd64",
  "config": {
    "Env": [
      "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
    ],
    "Cmd": [
      "bash"
    ],
-    "OnBuild": null
  },
  "created": "2023-01-10T12:34:56Z",
  "history": [
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "/bin/sh -c #(nop) ADD file:e2398d0bf516084b2b37ba1bb76b86d56e66999831df692461679fbd6a5d8eb6 in / "
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "/bin/sh -c #(nop)  CMD [\"bash\"]",
      "empty_layer": true
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c touch /foo # buildkit",
      "comment": "buildkit.dockerfile.v0"
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c touch /foo.1 # buildkit",
      "comment": "buildkit.dockerfile.v0"
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c touch -d '2010-01-01 12:34:56' /foo-2010 # buildkit",
      "comment": "buildkit.dockerfile.v0"
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c touch -d '2010-01-01 12:34:56' /foo-2010.1 # buildkit",
      "comment": "buildkit.dockerfile.v0"
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c touch -d '2030-01-01 12:34:56' /foo-2030 # buildkit",
      "comment": "buildkit.dockerfile.v0"
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c touch -d '2030-01-01 12:34:56' /foo-2030.1 # buildkit",
      "comment": "buildkit.dockerfile.v0"
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c rm -f /foo.1 # buildkit",
      "comment": "buildkit.dockerfile.v0"
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c rm -f /foo-2010.1 # buildkit",
      "comment": "buildkit.dockerfile.v0"
    },
    {
      "created": "2023-01-10T12:34:56Z",
      "created_by": "RUN /bin/sh -c rm -f /foo-2030.1 # buildkit",
      "comment": "buildkit.dockerfile.v0"
    }
  ],
  "os": "linux",
  "rootfs": {
    "type": "layers",
    "diff_ids": [
      "sha256:67a4178b7d47beb6a1f697a593bd0c6841c67eb0da00f2badefb05fd30671490",
-      "sha256:fd3a8c84d66d1161944ba081bf79e01c5a5f40bd406954fa75b6c19de4fe3705",
-      "sha256:098cf3f4823e8e21d65f85d834b381f78e966d1f538b42c86234db43c0ff97d6",
+      "sha256:ea8a8eb016286754093c35dca6d5b32286daa99e98bccf528666e30eed56af64",
+      "sha256:ad2fa1de61a9da3a0390cc637892c5bf5444c5bfc7728fcf9880232febba2f66",
      "sha256:9c203430ff59e4b967700fcde46d38dcaf3efc873649158a45df94ae14fb7502",
      "sha256:7650ee25e58bcdb5bb66c2171aeb5d4fea323dff34ec84e653472eb7f4c59334",
      "sha256:264f170d83b95f72e20a1138c6b31a46671115370f4e11316ebca9da0fd07d38",
      "sha256:cd9cf207cd02cbffd37427312117ba2df75387dd8416d24bbfc767f70d17d9d1",
      "sha256:d937a741ce6c0f1d15497c88f9452f5c3dde403c77f8b6ce1c30e4a21eef9cb7",
      "sha256:9447ede59b6226cfab0c72780c6c1a2b61d04c54110904c7c1636c11e94561ee",
      "sha256:890275bb26195ac894885324d69018c500eb176a1bc2d99d42e87f0a3d370806"
    ]
  }
}

The OnBuild change is expected because in our go structs it has the omitempty annotation whereas the Buildkit one did not. But not sure where the layer change comes from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did a quick try and changed the existing code to use omitempty to see if that's the only difference causing the issues; #4648

@thaJeztah thaJeztah force-pushed the use_docker_spec branch 3 times, most recently from c4d1f9c to ffb744f Compare February 17, 2024 14:21
@thaJeztah thaJeztah marked this pull request as ready for review February 17, 2024 14:22
@thaJeztah
Copy link
Member Author

Moved out of draft, because #4648 was merged

@tonistiigi @crazy-max PTAL

The Docker image specification (which extends the OCi types with
additional fields) was moved to a separate module, so we can now
use those definitions as a central place to define these types.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

@tonistiigi @AkihiroSuda ptal 🤗

@tonistiigi tonistiigi merged commit da55716 into moby:master Feb 20, 2024
67 checks passed
@thaJeztah thaJeztah deleted the use_docker_spec branch February 20, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants