FIXME: pass the logger and print the warning inside

[anushkamittal20]

References: issue
slack thread

I have made changes to remove the message returned from DecodeLayerMetadataFile and the consequent changes that were required.

[anushkamittal20]

@natalieparellano I started working on this issue, and I was hoping to get suggestions on how to tackle this.
Thank you very much!

[natalieparellano]

@anushkamittal20 this looks great! Apologies for the slow reply (I've been out). You are definitely on the right track! It would be good to add a couple tests in layermetadata_test.go. You can construct a "spy" logger for this, if you search for assertLogEntry you'll find where we do something similar elsewhere.

[anushkamittal20]

Thank you very much for the review @natalieparellano.
I will try to implement the same and push the changes!

[anushkamittal20]

@natalieparellano , sorry it took a bit, I have tried to implement the change, and the current implementation works as well. Please let me know if you need any corrections. Thank you for taking the time to help!
I also wanted to confirm how we can tackle this . Since we will no longer return the message string, there is a part of the code I have commented.

if msg != "" {
 	if api.MustParse(l.api).LessThan("0.6") {
 		l.logger.Warn(msg)
 	} else {
 		return LayerMetadata{}, errors.New(msg)
 	}
 } 

Thank you!

[jjbustamante]

@anushkamittal20 I think you forgot to run make format before pushing your code and the CI failed, just do that, run make format in your local environment and then push the changes.

[natalieparellano]

@anushkamittal20 I think (though I'm not sure) that the logic you quoted should move inside DecodeLayerMetadataFile around where we call decoder.Decode in that function. We have the buildpack API at that point, so we can log or return an error depending on the version.

[anushkamittal20]

@jjbustamante, I will run make format and push it. Thank you for letting me know!
@natalieparellano, I will try to move the logic in DecodeLayerMetadataFile and see if that works all right, thank you for your feedback. That was helpful!
P.S. Sorry for the delay, I will implement these changes and push them to the branch.

[natalieparellano]

Looks like it needs another make format

[natalieparellano]

@anushkamittal20 any update on this one?

[anushkamittal20]

@natalieparellano wanted to update you on this.
The required changes are done.
I was facing dependency issues on my OS and wasn't able to run make format I am currently trying to resolve the issues or work around them. Will push the new changes ASAP.

[natalieparellano]

Suggested change

	logger.Warn(str)
	if !api.MustParse(buildpackAPI).LessThan("0.6") {
	return LayerMetadataFile{}, errors.New(str)
	}
	if api.MustParse(buildpackAPI).LessThan("0.6") {
	logger.Warn(str)
	} else {
	return LayerMetadataFile{}, errors.New(str)
	}

I think this is what we want... the test failures may be due to a test needing to be updated

[natalieparellano]

Suggested change

	it("metadata file in wrong format", func() {
	it("logs a warning when the metadata file has wrong format (on older apis)", func() {

[natalieparellano]

We can make another it block for returns an error when the metadata file has wrong format that provides a newer buildpack API on the equivalent of line 59

[natalieparellano]

Suggested change

	lmf, _ = buildpack.DecodeLayerMetadataFile(metadataFile.Name(), "0.9", logger)
	lmf, err := buildpack.DecodeLayerMetadataFile(metadataFile.Name(), "0.5", logger)
	h.AssertNil(t, err)

[natalieparellano]

@anushkamittal20 thanks for taking another look at this. I think it's on the right track, we just need two different test cases for old and new buildpack API.

[anushkamittal20]

Hey @natalieparellano, thank you very much for your review.
I will work on the suggested changes and get back to you!

[natalieparellano]

Looks great! Awesome work @anushkamittal20 - thanks so much for the PR ❤️