Validate stack during analyze phase #617

jabrown85 · 2021-05-12T22:59:23Z

During analyze, Platform 0.7 and above will validate the build and stack image if the data is available to do so.

Addresses Part of: #471

Spec being implemented:

The lifecycle MUST fail if the stack ID of the <run-image> does not match <stack-id>

Signed-off-by: Jesse Brown jabrown85@gmail.com

jabrown85 · 2021-05-12T23:11:19Z

oh jeez - nevermind . this doesn't address #471 yet - I thought that ticket was about making sure build and run images match..but maybe we do this anyway since it is related? 😬

The buildpack.toml check I can work on, but it is a different phase since you only want to validate for the selected group I assume?

jabrown85 · 2021-05-13T13:09:22Z

I put in this fix while working on this. buildpacks/spec#225

jabrown85 · 2021-05-18T16:59:57Z

@micahyoung I would love your assistance on fixing windows tests. I added skips for the 6 tests I would expect to pass. I am building platform specific run images but I was still seeing failures as if the run image wasn't in the registry. Is there something I need to do to be able to push a windows image into the ggcr registry? If you have any thoughts to try out let me know!

https://github.com/buildpacks/lifecycle/pull/617/checks?check_run_id=2612523589

micahyoung · 2021-05-18T17:06:17Z

@jabrown85 Happy to take a look, as I know this is dragging on for ya. I'll see if I can get your current branch passing locally and send you a patch.

codecov · 2021-05-18T17:10:24Z

Codecov Report

Merging #617 (c3697ee) into main (8e9b27e) will increase coverage by 1.63%.
The diff coverage is 81.64%.

❗ Current head c3697ee differs from pull request most recent head cc21037. Consider uploading reports for the commit cc21037 to get more accurate results

@@            Coverage Diff             @@
##             main     #617      +/-   ##
==========================================
+ Coverage   64.90%   66.52%   +1.63%     
==========================================
  Files          52       60       +8     
  Lines        3623     3844     +221     
==========================================
+ Hits         2351     2557     +206     
+ Misses       1023     1020       -3     
- Partials      249      267      +18

Flag	Coverage Δ
os_linux	`68.32% <81.82%> (?)`
os_windows	`64.96% <81.64%> (+0.07%)`	⬆️
unit	`68.32% <81.82%> (?)`

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

natalieparellano · 2021-05-19T21:13:40Z

Did we answer the question of whether stack.toml should be required?

acceptance/analyzer_test.go

cmd/lifecycle/analyzer.go

stack_validation.go

stack_validation_test.go

utils_test.go

yaelharel · 2021-06-04T18:53:36Z

utils_test.go

@@ -145,4 +146,38 @@ func testUtils(t *testing.T, when spec.G, it spec.S) {
 			}
 		})
 	})
+
+	when(".ResolveRunImage", func() {


Please see https://github.com/buildpacks/lifecycle/pull/617/files#r645779179.

If we deleted this . here, I think it's worth removing the . in the other when in this file.

ReadOrder

ReadGroup

WriteTOML

TruncateSha

Do we want to remove .? I did it because it was requested, but I also am fine with leaving them and matching the style of the test that already exist. That is why I used the . in the first place...

I'm not sure. I think this is the only file that is using . and I'm not sure why.
Is there a reason for it?

🤷 - I've seen this pattern before for denoting public method tests. #Method or .Method

I am happy to remove them all or match the style of the file

But we're usually testing public methods and I have never seen this style anywhere else, but maybe I'm wrong and I just missed it.
Anyway, my first thought was about removing it but I don't have a strong opinion about it.

Just curious, when do we use . vs #? (I've seen "object method" vs "static method" in other languages but I'm not sure what is our convention.) That said I don't have an opinion here.

acceptance/analyzer_test.go

stack_validation_test.go

utils_test.go

acceptance/analyzer_test.go

acceptance/testdata/analyzer/analyze-image/Dockerfile

acceptance/analyzer_test.go

acceptance/testdata/analyzer/analyze-image/container/cnb/bad-stack.toml

acceptance/testdata/analyzer/analyze-image/container/cnb/local-bionic-stack.toml

acceptance/testdata/analyzer/analyze-image/container/cnb/missing-run-image.toml

acceptance/analyzer_test.go

acceptance/testdata/analyzer/analyze-image/Dockerfile

acceptance/analyzer_test.go

natalieparellano · 2021-06-10T21:40:52Z

This is looking great! The acceptance tests require a certain amount of concentration and navigating around to follow, so I added some suggestions for comments that might make it easier to understand.

jabrown85 · 2021-06-11T20:56:16Z

@natalieparellano I'd like to squash everything so I can more easily fix the conflict on main. Let me know when you've had a chance to review the later commits. I don't want to take away your ability to review easily

natalieparellano · 2021-06-14T15:29:33Z

@natalieparellano I'd like to squash everything so I can more easily fix the conflict on main. Let me know when you've had a chance to review the later commits. I don't want to take away your ability to review easily

@jabrown85 go for it!

During analyze, Platform 0.7 and above will validate the build and stack image if the data is available to do so. Addresses: #471 Signed-off-by: Jesse Brown <jabrown85@gmail.com>

natalieparellano

Do we need to add logic for the creator?

Forgot about the creator :/

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

jabrown85 · 2021-06-15T15:40:22Z

@natalieparellano creator should now be good to go after cc21037 and 55c17a5

utils.go

natalieparellano · 2021-06-16T14:20:46Z

utils.go

@@ -64,6 +66,30 @@ func DecodeLabel(image imgutil.Image, label string, v interface{}) error {
 	return nil
 }

+func ResolveRunImage(stackMD platform.StackMetadata, destinationImageRef string) (string, error) {


Could we add a docs comment here?

This reverts commit 7c5f36b, reversing changes made to 162090f.

Revert "Merge pull request #617 from buildpacks/jab/validate-stack"

jabrown85 self-assigned this May 12, 2021

jabrown85 requested a review from a team as a code owner May 12, 2021 22:59

jabrown85 force-pushed the jab/validate-stack branch from d630cc3 to f083d3d Compare May 17, 2021 14:22

natalieparellano reviewed May 19, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

acceptance/analyzer_test.go Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

cmd/lifecycle/analyzer.go Outdated Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

stack_validation.go Outdated Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

stack_validation.go Outdated Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

stack_validation_test.go Outdated Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

utils_test.go Outdated Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

utils_test.go Outdated Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

utils_test.go Outdated Show resolved Hide resolved

yaelharel reviewed Jun 4, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 4, 2021

View reviewed changes

stack_validation_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 4, 2021

View reviewed changes

stack_validation_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 4, 2021

View reviewed changes

utils_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 4, 2021

View reviewed changes

acceptance/analyzer_test.go Show resolved Hide resolved

natalieparellano reviewed Jun 4, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 4, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 4, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 4, 2021

View reviewed changes

acceptance/testdata/analyzer/analyze-image/Dockerfile Show resolved Hide resolved

jabrown85 force-pushed the jab/validate-stack branch 2 times, most recently from 5ec05dc to 3fa6b3a Compare June 8, 2021 17:10

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/testdata/analyzer/analyze-image/container/cnb/bad-stack.toml Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/testdata/analyzer/analyze-image/container/cnb/local-bionic-stack.toml Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/testdata/analyzer/analyze-image/container/cnb/missing-run-image.toml Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/testdata/analyzer/analyze-image/Dockerfile Outdated Show resolved Hide resolved

natalieparellano reviewed Jun 10, 2021

View reviewed changes

acceptance/analyzer_test.go Outdated Show resolved Hide resolved

jabrown85 force-pushed the jab/validate-stack branch 2 times, most recently from 27321c7 to 2cfc635 Compare June 14, 2021 15:42

Validate stack during analyze phase

efa1357

During analyze, Platform 0.7 and above will validate the build and stack image if the data is available to do so. Addresses: #471 Signed-off-by: Jesse Brown <jabrown85@gmail.com>

jabrown85 force-pushed the jab/validate-stack branch from 2cfc635 to efa1357 Compare June 14, 2021 16:07

natalieparellano previously approved these changes Jun 14, 2021

View reviewed changes

natalieparellano reviewed Jun 14, 2021

View reviewed changes

jabrown85 added 2 commits June 14, 2021 16:53

fixup! Validate stack during analyze phase

cc21037

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

fixup! Validate stack during analyze phase

55c17a5

Signed-off-by: Jesse Brown <jabrown85@gmail.com>

natalieparellano reviewed Jun 16, 2021

View reviewed changes

utils.go Show resolved Hide resolved

natalieparellano reviewed Jun 16, 2021

View reviewed changes

natalieparellano approved these changes Jun 16, 2021

View reviewed changes

natalieparellano merged commit 7c5f36b into main Jun 16, 2021

natalieparellano deleted the jab/validate-stack branch June 16, 2021 17:31

natalieparellano mentioned this pull request Jun 16, 2021

Update dependencies #628

Merged

natalieparellano added a commit that referenced this pull request Jun 24, 2021

Revert "Merge pull request #617 from buildpacks/jab/validate-stack"

b54bc9d

This reverts commit 7c5f36b, reversing changes made to 162090f.

natalieparellano added a commit that referenced this pull request Jun 24, 2021

Merge pull request #644 from buildpacks/revert-stack-id

8eda858

Revert "Merge pull request #617 from buildpacks/jab/validate-stack"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate stack during analyze phase #617

Validate stack during analyze phase #617

jabrown85 commented May 12, 2021 •

edited

jabrown85 commented May 12, 2021

jabrown85 commented May 13, 2021

jabrown85 commented May 18, 2021

micahyoung commented May 18, 2021

codecov bot commented May 18, 2021 •

edited

natalieparellano commented May 19, 2021

yaelharel Jun 4, 2021

yaelharel Jun 9, 2021

jabrown85 Jun 9, 2021

yaelharel Jun 9, 2021

jabrown85 Jun 9, 2021

yaelharel Jun 9, 2021

natalieparellano Jun 14, 2021

natalieparellano commented Jun 10, 2021

jabrown85 commented Jun 11, 2021

natalieparellano commented Jun 14, 2021

natalieparellano left a comment

jabrown85 commented Jun 15, 2021

natalieparellano Jun 16, 2021

Validate stack during analyze phase #617

Validate stack during analyze phase #617

Conversation

jabrown85 commented May 12, 2021 • edited

jabrown85 commented May 12, 2021

jabrown85 commented May 13, 2021

jabrown85 commented May 18, 2021

micahyoung commented May 18, 2021

codecov bot commented May 18, 2021 • edited

Codecov Report

natalieparellano commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natalieparellano commented Jun 10, 2021

jabrown85 commented Jun 11, 2021

natalieparellano commented Jun 14, 2021

natalieparellano left a comment

Choose a reason for hiding this comment

jabrown85 commented Jun 15, 2021

Choose a reason for hiding this comment

jabrown85 commented May 12, 2021 •

edited

codecov bot commented May 18, 2021 •

edited