-
Notifications
You must be signed in to change notification settings - Fork 104
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
Verify read access while selecting best run-image
mirror
#1024
Verify read access while selecting best run-image
mirror
#1024
Conversation
8fc3d99
to
3c002fb
Compare
9c0d441
to
3dc6f0a
Compare
3dc6f0a
to
b358a97
Compare
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.
@pbusko thanks for this! I think broadly this PR accomplishes what we want. I'm requesting changes to hopefully align the code with some recently introduced constructs (the image handler) and keep platform.LifecycleInputs
scoped to only the things that a platform (operator) would provide. Feel free to offer other suggestions if you think there is a better way to do it.
b358a97
to
615f926
Compare
@c0d1ngm0nk3y @pbusko apologies for the slow reply (I've been out). I'll have a look at your changes soon.
If the export target is the daemon, then we also expect to find the run image in a daemon. We don't allow "mixing and matching" although maybe (someday) we should. |
|
||
type SimpleImageStrategy struct{} | ||
|
||
var _ platform.ImageStrategy = &SimpleImageStrategy{} |
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 a bit confused - why do we need this global (and why do we override it since it lives on inputs.AccessChecker
)?
|
||
type RemoteImageStrategy struct{} | ||
|
||
var _ ImageStrategy = &RemoteImageStrategy{} |
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.
Curious why we need this global
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.
@natalieparellano Actually, it is used in the main package as well (rebaser.go
, analyzer.go
)
d4cf0e6
to
8742299
Compare
@natalieparellano Did you have a chance to have a closer look? Is something still missing from your side? |
@c0d1ngm0nk3y - many apologies, I missed your comment from 2 weeks ago prodding me to take another look 😞 I'll take a look today |
Looks good @c0d1ngm0nk3y! I think you need to run |
fac4012
to
c1256b4
Compare
Signed-off-by: Pavel Busko <pavel.busko@sap.com> Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com> Co-authored-by: Johannes Dillmann <j.dillmann@sap.com> Add unit tests for run image resolution Co-authored-by: Johannes Dillmann <j.dillmann@sap.com> Co-authored-by: Philipp Stehle <philipp.stehle@sap.com> Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com> Use ImageStrategy instead of AccessChecker to ease testing Signed-off-by: Johannes Dillmann <j.dillmann@sap.com> Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com> Signed-off-by: Johannes Dillmann <j.dillmann@sap.com> Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com> Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>
eb27966
to
8778ef5
Compare
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 acceptance tests failed, so we had to make another small adjustment. Can you check and approve the workflow again?
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 acceptance tests failed, so we had to make another small adjustment. Can you check and approve the workflow again?
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com> Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>
8778ef5
to
96843c8
Compare
@c0d1ngm0nk3y @pbusko apologies for the extended back and forth on this one - if you could just pull in https://github.com/buildpacks/lifecycle/pull/1024/files#r1188710899, if the tests all pass then we will merge this in. |
Co-authored-by: Natalie Arellano <narellano@vmware.com> Signed-off-by: Ralf Pannemans <ralf.pannemans@googlemail.com>
ab1dc96
to
c457a22
Compare
@natalieparellano There was a small oversight in the test, I fixed that. Sorry. The tests should be fine now. |
@@ -20,6 +20,12 @@ var ( | |||
) | |||
|
|||
func ResolveInputs(phase LifecyclePhase, i *LifecycleInputs, logger log.Logger) error { | |||
if i.UseDaemon || i.UseLayout { |
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 LocalimageStrategy
correct for Daemon run images in all cases?
I thought pack build <blah> --publish
, for instance, would read the run image from the target registry to place the app layers onto without pulling it down.
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.
Ah yes, but in the --publish
case i.UseDaemon
would be false
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.
According to this comment
If the export target is the daemon, then we also expect to find the run image in a daemon. We don't allow "mixing and matching" although maybe (someday) we should.
But does --publish
not imply that you export to a registry and not the daemon?
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 long as i.UseDaemon
is false I think we are good.
Green! Thanks for the PR @c0d1ngm0nk3y and @pbusko! |
@natalieparellano We would really like to consume this change. Do you know what the current plans for release |
@c0d1ngm0nk3y we are getting really close - at this point, we need the specs to be finalized and shipped before we can ship the lifecycle. You can track the progress here: #1165 At this point, I am hoping for a lifecycle 0.17.0-rc.4 early next week, and a lifecycle 0.17.0 late next week. I have a record of being overly optimistic, but I think that's achievable 🙏🏼 🤞🏼 |
I missed the mark by a couple of days, but |
Instead of simply picking the first run-image, the lifecycle will select the first accessible reference, if there's no match by registry.
See buildpacks/spec#357