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

Introduce new attribute on oci_image for created datetime #724

Merged
merged 6 commits into from
Oct 29, 2024

Conversation

hanneskaeufler
Copy link
Contributor

@hanneskaeufler hanneskaeufler commented Oct 18, 2024

Fixes #661 . Alternative to #722.

Why?

Doing a docker inspect --format='{{.Created}}' my/image:latest on an image built with oci_image previously always returned the static beginning of the unix timestamp (1. January 1970 00:00:00). To be more precise, that's what it returns for images that are built "from scratch". When an image is built on a base, then the created date is set to the created date of the base image.

Both are fine strategies for reproducible builds, but in stamped builds that are eventually shipped/deployed, you probably want to set that time to the actual build time.

What?

To allow setting that particular Created in the manifest json, we introduce a separate file label, allowing to pass in a file containing a stamp variable.

@hanneskaeufler
Copy link
Contributor Author

@alexeagle @thesayyn ping in case you haven't seen it. If you'll allow the CI to run I'll make sure to pass that :) Thanks for your time!

Copy link
Collaborator

@thesayyn thesayyn left a comment

Choose a reason for hiding this comment

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

Looks good to me, i wish this property was not just named created but something else like created_at

@thesayyn
Copy link
Collaborator

Oh did not mean that you should rename it, we should use the created property as that's what the OCI spec says.

@hanneskaeufler
Copy link
Contributor Author

Oh did not mean that you should rename it, we should use the created property as that's what the OCI spec says.

Sorry I don't follow. I feel created_at as an attribute here in rules_oci is indeed the better choice. In the json manifest of course I still keep created https://github.com/bazel-contrib/rules_oci/pull/724/files#diff-4d907c3439c162250ee007024daf813b7354ab62d3a510f68955ca9751d33618R173.

Let me know if that clears it up :) Thanks!

@thesayyn
Copy link
Collaborator

Let me know if that clears it up :) Thanks!

the name of the attribute should be identical to config property. so both of them should be created. i was merely suggesting that OCI spec should have named it differently but there is nothing we can do now :)

Copy link
Collaborator

@thesayyn thesayyn left a comment

Choose a reason for hiding this comment

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

some concerns on e2e, otherwise lgtm

@hanneskaeufler
Copy link
Contributor Author

some concerns on e2e, otherwise lgtm

Pushed everything I have now, should pass CI as well :) Thx again!

@hanneskaeufler hanneskaeufler changed the title Introduce new attribute for created datetime Introduce new attribute on oci_image for created datetime Oct 25, 2024
@thesayyn
Copy link
Collaborator

@hanneskaeufler could you rebase?

Doing a docker inspect --format='{{.Created}}' my/image:latest on an
image built with oci_image previously always returned the static
beginning of the unix timestamp (1. January 1970 00:00:00). To be more
precise, that's what it returns for images that are built "from
scratch". When an image is built on a base, then the created date is set
to the created date of the base image.

Both are fine strategies for reproducible builds, but in stamped builds
that are eventually shipped/deployed, you probably want to set that time
to the actual build time.

To allow setting that, we introduce a separate file label, allowing to
pass in a file containing a stamp variable.
Registers dependencies as per installation instructions.
It's not used and has portability issues
@hanneskaeufler
Copy link
Contributor Author

@thesayyn sure can, done ✅

@thesayyn thesayyn merged commit 4355c8b into bazel-contrib:main Oct 29, 2024
14 checks passed
@hanneskaeufler hanneskaeufler deleted the hk-created branch October 29, 2024 17:38
hanneskaeufler added a commit to hanneskaeufler/rules_oci that referenced this pull request Oct 29, 2024
https://github.com/bazel-contrib/rules_oci/actions/runs/11564798863/job/32190695142

```
Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
```

In bazel-contrib#724 it looks like I broke macos CI because that particular step is
not run as part of the PR builds. I introduced a genrule which requires
access to the docker daemon, while previously it was only used in test
rules.

Adding the env var to the docker host to the action graph should maybe
fix this?
hanneskaeufler added a commit to hanneskaeufler/rules_oci that referenced this pull request Oct 29, 2024
https://github.com/bazel-contrib/rules_oci/actions/runs/11564798863/job/32190695142

```
Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
```

In bazel-contrib#724 it looks like I broke macos CI because that particular step is
not run as part of the PR builds. I introduced a genrule which requires
access to the docker daemon, while previously it was only used in test
rules.

Adding the env var to the docker host to the action graph should maybe
fix this?
thesayyn pushed a commit that referenced this pull request Nov 5, 2024
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.

oci_image should support setting created and stamp
2 participants