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

read image pull response to save image to local cache #2282

Merged
merged 8 commits into from
Aug 4, 2022

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Aug 3, 2022

What does this change

Previously, we assumed that by pulling an image, the image is automatically cached by docker. However, it seems that the caching only happens when the response from pull is read.

This PR fixes above issue by reading the full response. I also changed the hello smoke test to always run in a fresh local docker environment to make sure this scenario is tested.

What issue does it fix

Closes #2280

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
tests/smoke/hello_test.go Outdated Show resolved Hide resolved
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
pkg/cnab/config-adapter/testdata/mybuns.bundle.json Outdated Show resolved Hide resolved
// Set up a fresh docker environment
mgx.Must(shx.RunV("docker", "system", "prune", "--all", "-f"))
// make sure the referenced image is not in local image cache
mgx.Must(shx.RunV("docker", "rmi", "carolynvs/whalesayd"))
Copy link
Member

Choose a reason for hiding this comment

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

If the image is not in the cache, this line will fail. In cases where you just need to do cleanup before a test, I suggest not checking the error message (kind of like calling rmdir and then ignoring the error if the directory doesn't exist).

@@ -110,7 +110,7 @@ func TestPorter_Build_ChecksManifestSchemaVersion(t *testing.T) {
defer p.Close()

// Make a bundle with the specified schemaVersion
p.TestConfig.TestContext.AddTestDirectoryFromRoot("tests/testdata/mybuns", "/")
p.TestConfig.TestContext.AddTestDirectoryFromRoot("tests/testdata/mydb", "/")
Copy link
Member

Choose a reason for hiding this comment

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

Can you help me understand why this was changed to user a different bundle? If the test worked before with mybuns, I would expect it to still work with mybuns with a referenced image. I am concerned that changing the test bundle may be hiding a regression or problem in Porter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it back

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
@VinozzZ
Copy link
Contributor Author

VinozzZ commented Aug 3, 2022

/azp run porter-integration

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Thanks for adding the caching to our TestRegistry. I realize it may not be what we want for the tests long term, but I appreciate that it preserves our existing regression tests.

@@ -65,5 +76,13 @@ func (t *TestRegistry) PullImage(ctx context.Context, image string) error {
if t.MockPullImage != nil {
return t.MockPullImage(ctx, image)
}
sum, err := NewImageSummary(image, types.ImageInspect{
ID: cnab.NewULID(),
RepoDigests: []string{fmt.Sprintf("%s@sha256:75c495e5ce9c428d482973d72e3ce9925e1db304a97946c9aa0b540d7537e041", image)},
Copy link
Member

Choose a reason for hiding this comment

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

I assume that this digest here is specifically the one that makes your test work for this PR. If we pulled multiple images using this newly added cache, each image would get the same digest, correct?

You don't need to change this, just something to keep in mind if we end up needing to use this TestRegistry for other tests. Hopefully we can move away from this type of mocking when we redo the integration tests to use the tester framework.

@VinozzZ VinozzZ merged commit f7aa99d into getporter:release/v1 Aug 4, 2022
@VinozzZ VinozzZ deleted the fix-image-cache branch August 4, 2022 14:43
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.

Airgapped images are broken in v1.0.0-beta.2
2 participants