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

Fixes error interpolation case where err is nil #289

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

ryanmoran
Copy link
Member

Summary

In cases where the checksums for dependencies don't match, we were outputting an error message that looked broken:

[builder]   Executing build process
[builder]     Installing Node Engine 16.13.2
[builder] checksum does not match: %!s(<nil>)

This was caused by our attempt to interpolate nil as a string in the error message. We don't need this interpolation because we have already checked that the error is nil in prior statements.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@ryanmoran ryanmoran requested a review from a team as a code owner February 23, 2022 18:51
@@ -616,7 +616,7 @@ version = "this is super not semver"
"",
)

Expect(err).To(MatchError(ContainSubstring("checksum does not match")))
Copy link
Member

Choose a reason for hiding this comment

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

This test is testing

packit/postal/service.go

Lines 172 to 175 in 86f0343

err = vacation.NewArchive(validatedReader).WithName(name).StripComponents(dependency.StripComponents).Decompress(layerPath)
if err != nil {
return err
}
I think. Is that intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I missed that we had an earlier test that asserts we handle the error coming back from the decompression. Unfortunately, I've been unable to create a test case that covers what I really want to test, which is the validatedReader.Valid() conditional. I can't seem to create a tarball that triggers the situation I am able to see in a real dependency in a real buildpack.

Copy link
Contributor

Choose a reason for hiding this comment

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

What buildpacks are you seeing this error in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have constructed a buffer that can get to the second failure state. https://gist.github.com/ForestEckhardt/84eb2305c4626e3b93a5457baed20764

@sophiewigmore sophiewigmore added the semver:patch A change requiring a patch version bump label Feb 23, 2022
@sophiewigmore sophiewigmore merged commit 99c168f into v2 Feb 23, 2022
@sophiewigmore sophiewigmore deleted the fix-error-interpolation branch February 23, 2022 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants