Update run image metadata when --run-image is provided #2022

Pratham1812 · 2024-01-11T08:56:12Z

Summary

Output

Before

After

Documentation

Should this change be documented?
- Yes, see #___
- No

{
  "runImage": {
    "topLayer": "sha256:be9eb0a7d50b0438c62376a29b5557f85496a55ae83d41a49f934160ffa09421",
    "reference": "945a5e4052d9b9b024f69bf89f8cf8bd72bc727882ab0b78df9592b37119a31c",
    "image": "paketobuildpacks/run-jammy-base:0.0.2"
  },
  "stack": {
    "runImage": {
      "image": "paketobuildpacks/run-jammy-base:0.0.2"
    }
}

Is it ok to change the stack.runImage.image? I think we shouldn't, we should only change the runImage.image metadada

natalieparellano · 2024-01-22T21:43:12Z

Is it ok to change the stack.runImage.image? I think we shouldn't, we should only change the runImage.image metadada

It should be okay, actually preferred - runImage.image and stack.runImage.image refer to the same thing, and are written twice for backwards compatibility.

jjbustamante · 2024-02-08T23:01:42Z

Hi @Pratham1812 could try to solve the merge conflicts and the DCO check ?

internal/builder/builder.go

jjbustamante · 2024-03-04T23:08:08Z

I tried to fix the acceptance test failures. I focused in one of them and here are my findings.

The test case is trying to rebase an application image saved in the docker daemon with mirrors metadata saved with the application image.

Behavior before this PR

First we created a builder, and the builder has a pack-test/run as run image
Then we built the application image with a command similar to pack build localhost:32801/some-org/xeyagowjzc --run-image localhost:32801/run-before/vgdpkyhhne
The output from the previous command is an application image localhost:32801/some-org/xeyagowjzc with the following io.buildpacks.lifecycle.metadata

"runImage": {
    "topLayer": "sha256:47746006a138ccf6c120beaf5ec55a5589fc6a9b93777082ee9e3b722f3ddf73",
    "reference": "92761540a511123b9c76b9e4fe8c4826c3d19ef1dbfd67ed6b825d6b43546bb6",
    "image": "pack-test/run",
    "mirrors": [
      "localhost:32801/pack-test/run"
    ]
  },
  "stack": {
    "runImage": {
      "image": "pack-test/run",
      "mirrors": [
        "localhost:32801/pack-test/run"
      ]
    }
  }

Finally we run pack rebase localhost:32801/some-org/xeyagowjzcand and the Test expects the run image to be selected from the mirror.

If we take a look at the previous metadata, the runImage.Image is using the value from the Builder and the --run-image used during pack build is not being saved (which is actually the issue this PR is trying to solve)

Behavior with this PR

The current PR is updating the metadata when pack build is invoked, as I mentioned above, we are running pack build --run-image to setup the test fixture. The io.buildpacks.lifecycle.metadata looks like this with the new code.

"runImage": {
    "topLayer": "sha256:74275d5fce8e3c3343321b75529e7751bd5966664f6af3aec1bb50890c7b6e39",
    "reference": "5d17ab3bdb1b4764f429b059f7a8a1a7f820c4553d2a3f3dfeff69668a5c881a",
    "image": "localhost:32804/run-before/byxrrrztji",
    "mirrors": [
      "localhost:32804/pack-test/run"
    ]
  },
  "stack": {
    "runImage": {
      "image": "localhost:32804/run-before/byxrrrztji",
      "mirrors": [
        "localhost:32804/pack-test/run"
      ]
    }
  }

When running the rebase, the run image selected is localhost:32804/run-before/byxrrrztji and not the expected localhost:32804/pack-test/run.
I think the metadata is correct, and the problem is with the test case and how to set up the fixture to use the mirrors. I think we need to do something to make the access_checker to return false when run-before image is used, I tried removing the image in the Before but it didn't work

natalieparellano · 2024-03-05T15:02:05Z

internal/builder/builder.go

@@ -140,6 +148,11 @@ func constructBuilder(img imgutil.Image, newName string, errOnMissingLabel bool,
 		return nil, err
 	}

+	if opts.runImage != "" {
+		metadata.RunImages = []RunImageMetadata{{Image: opts.runImage}}
+		metadata.Stack.RunImage.Image = opts.runImage


Looking at this again, we need to be consistent in how we handle mirrors information. Right now it is being zeroed out for metadata.RunImages but preserved for metadata.Stack.RunImage (as metadata.Stack.RunImage.Mirrors could still have data).

I propose we look in the existing known run images (RunImageMetadata.Mirrors) and if the provided opts.runImage is found there then preserve the run image mirror information, otherwise zero it out.

Yeah, I did that locally, trying to fix the issues with the acceptance but I didn't push it

natalieparellano · 2024-03-05T15:12:06Z

@jjbustamante since the linked acceptance test is validating rebase behavior we should probably just update the test setup to produce a test image (repoName) with the correct metadata. I think if we update these lines in the test setup to remove --run-image it might just work, as this particular test doesn't have any expectations around runBefore. We might have to duplicate the setup for this test to avoid breaking others in this block.

Update: I think my suggestion of updating the setup will work. But, you have to also remove these lines from the setup, otherwise the test will fail with an inscrutable error.

codecov · 2024-03-05T21:11:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.72%. Comparing base (6a9e3d1) to head (dfe0e73).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2022      +/-   ##
==========================================
+ Coverage   79.70%   79.72%   +0.02%     
==========================================
  Files         176      176              
  Lines       13250    13261      +11     
==========================================
+ Hits        10560    10571      +11     
  Misses       2021     2021              
  Partials      669      669

Flag	Coverage Δ
os_linux	`78.63% <100.00%> (+0.01%)`	⬆️
os_macos	`76.36% <100.00%> (+0.02%)`	⬆️
os_windows	`79.11% <100.00%> (+0.02%)`	⬆️
unit	`79.72% <100.00%> (+0.02%)`	⬆️

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

internal/builder/builder_test.go

jjbustamante · 2024-03-06T17:43:46Z

Hi @Pratham1812!

We made some changes to your code to get it into the last mile! but we need you to fix the DCO issues to get this PR merged. Is it possible for you to do that?

jjbustamante · 2024-03-07T22:15:47Z

@Pratham1812

The steps required to sign-off your commits are describe here, the most important are:

To add your Signed-off-by line to every commit in this branch:

Ensure you have a local copy of your branch by checking out the pull request locally via command line.
In your local branch, run: git rebase HEAD~16 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin alpha/patch

It is very important that you sign-off your commits in order for us to merge the PR! Otherwise we can't do it

jjbustamante · 2024-03-11T21:12:57Z

Oh man! I think you didn't update your main fork before running the rebase right? and we still have some commits without signing.

Signed-off-by: Pratham Agarwal <agarwalpratham1812@gmail.com>

- Adding acceptance tests Signed-off-by: Juan Bustamante <jbustamante@vmware.com>

internal/builder/builder_test.go

acceptance/assertions/image.go

acceptance/acceptance_test.go

natalieparellano · 2024-04-05T20:45:24Z

Looks good to me :) I suggested a few cosmetic changes

…ck from Natalie Signed-off-by: Juan Bustamante <jbustamante@vmware.com>

Pratham1812 requested review from a team as code owners January 11, 2024 08:56

github-actions bot added this to the 0.33.0 milestone Jan 11, 2024

github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jan 11, 2024

natalieparellano reviewed Jan 17, 2024

View reviewed changes