-
Notifications
You must be signed in to change notification settings - Fork 691
Add digests of the images being used for examples #1964
Add digests of the images being used for examples #1964
Conversation
@@ -43,20 +43,23 @@ _java_image_repos() | |||
|
|||
container_pull( |
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.
@gregmagolan this looks like a chance for us to document that surprising behavior that the tag is ignored if a digest is present, do I have that right?
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.
It's documented as part of the tag
attribute for container_pull
.
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.
could you just add a comment on the tag
field that it is now ignored, and only useful as documentation?
Might help some user who is following the example, as this is a serious footgun.
(we had a client update the tag, probably thinking they picked up a security fix, and did not realize that it's a no-op. IMO we should actually error in this case 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.
@alexeagle Just want to confirm the above comment is for me?
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.
yes please, I'd rather not introduce digests into the example unless we warn users about that
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 believe @alexeagle is referring to commenting in the BUILD file as the current behaviour is suprising:
name = "alpine_linux_amd64",
digest = "sha256:954b378c375d852eb3c63ab88978f640b4348b01c1b3456a024a81536dafbbf4",
registry = "index.docker.io",
repository = "library/alpine",
# tag field is ignored since digest is set
tag = "3.8",
I also think it should be an error if you specify both since it is too easy to update the tag and think that your change is load bearing
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 was imagining that we should error when you provide both digest
and tag
since the latter is ignored, but @gravypod points out that bazel's git_repository
does the same. I guess we could warn but it's not actionable. We could document and recommend the separate dictionary scheme that @gregmagolan did for a client. WDYT?
To merge this PR I'm happy with any comment on the tag
field so users are aware.
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.
Thanks!
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
In
testing/examples
, runningbazel test //...
yields the following warnings that digests of the images being used are not specified:Issue Number: N/A
What is the new behavior?
Specifies the digests suggested by the warnings.
Does this PR introduce a breaking change?
Other information