-
Notifications
You must be signed in to change notification settings - Fork 71
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
RFC for the export report: report.toml #70
Conversation
Signed-off-by: Matthew McNew <mmcnew@pivotal.io>
ceaaa62
to
0e8b010
Compare
Here is an overview of the suggested schema for report.toml: | ||
|
||
``` | ||
[image] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in the CNB group meeting, we may want to consider making this a bit more future proof to support multiple image output from a single build.
text/0000-export-report.md
Outdated
# How it Works | ||
[how-it-works]: #how-it-works | ||
|
||
The export stage will write report.toml after exporting and caching is complete. There is no default path for report.toml as its location will be platform-specific. If the export steps fails to complete no report.toml is expected to be written. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to think this is generally useful for most platforms. I'm a big fan of setting a default path that could be overrideable instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest ./report.toml
(or ./.toml) (similar to ./group.toml
and ./analyzed.toml
default paths) which generally resolves to /layers/report.toml
given that the working dir is usually the layers dir.
We should call out that the default can be changes with an env var or overrode with an exporter flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to default to the working directory.
text/0000-export-report.md
Outdated
|
||
- How/if `pack` should expose this information to users in a programmatic way? | ||
- Is there any additional metadata that should be provided in report.toml? | ||
- Is report the best name for this file and should it be marshaled into toml? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export.toml
?
If this is an export report, could it include the layer usage/size/reuse(?) without needing to dig into the docker image itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we have an analyzed.toml
, maybe exported.toml
is the most consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like exported.toml
. Updated the rfc.
Based on the Spec Changes RFC, do we need to add a |
- This is more consistent with the existing analyzed.toml - Default `exported.toml` to default into the working dir to be consistent with analyzed.toml Signed-off-by: Matthew McNew <mmcnew@pivotal.io>
7720be8
to
291305e
Compare
text/0000-export-report.md
Outdated
``` | ||
[image] | ||
tags = ["index.docker.io/image/name:latest", "index.docker.io/image/name:other-tag"] | ||
identifier = "sha256:c8be1b8f4d60d99c281fc2db75e0f56df42a83ad2f0b091621ce19357e19d853" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
identifier
appears to be generic. It would be nice to know if it's an Image ID (in the daemon case) or Digest (in the case of a registry).
Adding this as a separate comment. This might be scope creep, but does it make sense to include other image/layer information like the layer usage/size/reuse(?) without needing to dig into the docker image itself. |
Working group discussion on April 16th:
Please let me know if I missed anything or misrepresented any part of our discussion. |
- Add a bit of info about what should go in report.toml and what it should be used for. - Add docker daemon and registry schema examples - Docker Daemon builds will use the key image_id - Registry builds will use the key digest Signed-off-by: Matthew McNew <mmcnew@pivotal.io>
Do you have thoughts on what layer info might look like in report.toml? Perhaps, something like this? [[layers]]
buildpack = "some_buildpack/ruby"
name = "mri"
on_export = "reused"
cached = true
launch = true
build = true
size = 17813
[[layers]]
buildpack = "some_buildpack/ruby"
name = "cache"
on_export = "cached"
cached = true
launch = false
build = false
size = 64811 Update: This additional layer metadata is out of scope for this RFC. |
# Unresolved Questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
- How/if `pack` should expose this information to users in a programmatic way? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to resolve this before accepting the RFC or is that just a separate RFC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is primarily requested to enable workflows in kubernetes like platforms that invoke the lifecycle directly, I would like to push that to a separate rfc if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jromero thoughts on inclusion in pack
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only case in which we'd be likely to use this would be for something like the quiet
option proposed where we'd silence the output of the lifecycle and only use the information provided by this file to output only the desired information. (This could have also been achieved via structured logging. 😄)
I would agree with @matthewmcnew that pack is not the primary target for this feature and hence fleshing that out isn't necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewmcnew can you put in here that this question is out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to indicate that this is out of scope for this RFC
text/0000-export-report.md
Outdated
[unresolved-questions]: #unresolved-questions | ||
|
||
- How/if `pack` should expose this information to users in a programmatic way? | ||
- Is there any additional metadata that should be provided in report.toml? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to see the layer size info and like the toml structure you laid out. I can also see the argument of making that a separate RFC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't feel strongly about adding the layer sizing stuff in here, feel free to mark this as out of scope too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to indicate that this is out of scope for this RFC
@matthewmcnew Some of the proposed metadata additions here may over with #63, which solves part of the problem by adding all layer metadata to the exported image even if |
Signed-off-by: Matthew McNew <mmcnew@pivotal.io>
Entering Final Comment Period for merge disposition, closing 23 May, 2020. |
image_id = "9c0e1895b90f" | ||
``` | ||
|
||
By representing this in toml, we can extend this information with additional metadata when needed. A general philosophy for report.toml is that it should contain information that is not accessible elsewhere. For example, the bill of materials should not be included as it is available as an image label and would be unnecessarily duplicated in report.toml. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any decent tooling for extracting toml
keys? Something like jq
for toml
?
Suppose what I wanted in a particular result file (e.g. in Tekton) was the image digest, what is the guidance for extracting particular keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use yj with jq. yj -t | jq -r .image.digest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we've been doing in the heroku buildpacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, TIL.
Readable