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

Fix rebase run-image resolution #1305

Conversation

ryanbrainard
Copy link
Contributor

@ryanbrainard ryanbrainard commented Feb 29, 2024

Summary

Currently, if -run-image is not set, io.buildpacks.lifecycle.metdata[runImage.reference] is used. This does not follow the Run Image Resolution spec, which specifies using io.buildpacks.lifecycle.metdata[runImage.image] and optionally io.buildpacks.lifecycle.metdata[runImage.mirrors]. Because of this, it ends up making lifecycle rebase without the -run-image flag a no-op because the run image is pinned to the same version instead of getting the latest.

This change simplifies and unifies the behavior for before and after Platform Version 0.12 so that they both read the same run-image data type (just from different locations), and then validate and resolve mirrors the same.

Note, this is also consistent with the way pack rebase resolves run-images; however, it does not switch on the Platform Version (I didn't want to change that here).

Release notes

Fix lifecycle rebase automatic run image resolution from io.buildpacks.lifecycle.metdata to read keys runImage.image (and optionally runImage.mirrors) instead of runImage.reference.

Context

I did not see any relevant tests for this code, but please let me know if there's a test I should change or add. I did do various permutations of manual testing and it is working as expected.

Currently, if `-run-image` is not set, `io.buildpacks.lifecycle.metdata[runImage.reference]` is used. This does not follow the [Run Image Resolution spec](https://github.com/buildpacks/spec/blob/main/platform.md#run-image-resolution), which specifies using `io.buildpacks.lifecycle.metdata[runImage.image]` and optionally `io.buildpacks.lifecycle.metdata[runImage.mirrors]`. Because of this, it ends up making `lifecycle rebase` without the `-run-image` flag a no-op because the run image is pinned to the same version instead of getting the latest.

This change simplfies and unifies the behavior for before and after Platform Version 0.12 so that they both read the same run-image data type (just from different locations), and then validate and resolve mirrors the same.

Signed-off-by: Ryan Brainard <966764+ryanbrainard@users.noreply.github.com>
@ryanbrainard ryanbrainard requested a review from a team as a code owner February 29, 2024 18:54
This is to make go-critic happy. See go-critic/go-critic#453.

Signed-off-by: Ryan Brainard <966764+ryanbrainard@users.noreply.github.com>
@ryanbrainard ryanbrainard force-pushed the rb/rebase-fix-run-image-resolution branch from 450874f to b1b3fa5 Compare February 29, 2024 19:30
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

Thank you for this. It would be great if we could test this somehow - an acceptance test would probably be too heavy handed, but perhaps some of this logic could move to https://github.com/buildpacks/lifecycle/blob/main/platform/resolve_inputs.go (adding a function similar to here)

@ryanbrainard
Copy link
Contributor Author

Thank you for this. It would be great if we could test this somehow - an acceptance test would probably be too heavy handed, but perhaps some of this logic could move to https://github.com/buildpacks/lifecycle/blob/main/platform/resolve_inputs.go (adding a function similar to here)

Yes, I would love to add a test. I just looked at resolve_inputs.go, but that doesn't look like it'll work because this needs to happen after input resolution because of some caveots with -daemon (according to the comments). Looks like platform/run_image.go would be a good place where I can at least extract out the switch statement and put some tests around it. I'll push something up soon.

Signed-off-by: Ryan Brainard <966764+ryanbrainard@users.noreply.github.com>
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.62%. Comparing base (f885774) to head (5a55111).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1305      +/-   ##
==========================================
+ Coverage   64.57%   64.62%   +0.06%     
==========================================
  Files         101      101              
  Lines        6984     6994      +10     
==========================================
+ Hits         4509     4519      +10     
  Misses       2064     2064              
  Partials      411      411              
Flag Coverage Δ
os_linux 64.10% <100.00%> (+0.06%) ⬆️
os_windows 56.68% <100.00%> (+0.07%) ⬆️
unit 64.10% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@ryanbrainard
Copy link
Contributor Author

@natalieparellano I added a test in 5a55111

@natalieparellano natalieparellano merged commit 73af3c1 into buildpacks:main Mar 1, 2024
10 checks passed
@ryanbrainard ryanbrainard deleted the rb/rebase-fix-run-image-resolution branch March 1, 2024 14:48
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.

3 participants