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

upgrading some lifecycle api versions #1254

Conversation

joe-kimmel-vmw
Copy link

No description provided.

Signed-off-by: Joe Kimmel <jkimmel@vmware.com>
Signed-off-by: Joe Kimmel <jkimmel@vmware.com>
@joe-kimmel-vmw joe-kimmel-vmw changed the title maybe upgrading some lifecycle api versions upgrading some lifecycle api versions Jul 12, 2023
@joe-kimmel-vmw joe-kimmel-vmw marked this pull request as ready for review July 12, 2023 22:14
@joe-kimmel-vmw joe-kimmel-vmw requested a review from a team as a code owner July 12, 2023 22:14
@@ -903,34 +762,11 @@ func testBuildPod(t *testing.T, when spec.G, it spec.S) {
}
assert.Equal(t, append(append([]string{
"-layers=/layers",
"-analyzed=/layers/analyzed.toml",
"-run-image=builderregistry.io/run"},
Copy link
Author

Choose a reason for hiding this comment

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

I think this is correct that the run-image wouldn't be there anymore, because we've upgraded the platform API version, and think that really is the main change, but uh... cc @natalieparellano :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We should still provide -run-image to the analyzer. It is only removed as a flag to the exporter on newer platforms.

pkg/apis/build/v1alpha2/build_pod.go Show resolved Hide resolved
pkg/apis/build/v1alpha2/build_pod.go Outdated Show resolved Hide resolved
pkg/apis/build/v1alpha2/build_pod.go Outdated Show resolved Hide resolved
…form apis

Signed-off-by: Joe Kimmel <jkimmel@vmware.com>
Comment on lines 272 to 274
func() []string {
if !platformAPILessThan07 {
return []string{"-run-image=" + runImage}
}
return []string{}
}(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This func, along with L276-L278 can be removed. The args func just appends a bunch of string slices together, so if we return an empty slice, it's a noop either way.

Comment on lines +300 to +301
},
[]corev1.VolumeMount{}),
Copy link
Contributor

Choose a reason for hiding this comment

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

The volumeMounts func does the same thing as args, so passing in an empty slice is noop

Suggested change
},
[]corev1.VolumeMount{}),
}),

return []string{}
}(),
[]string{fmt.Sprintf("-report=%s", ReportTOMLPath)},
[]string{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty slice is noop

Suggested change
[]string{},

} else {
return []string{fmt.Sprintf("-process-type=web")}
}
return []string{fmt.Sprintf("-process-type=web")}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return []string{fmt.Sprintf("-process-type=web")}
return nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, platformAPI.GreaterThan(semver.MustParse("0.5")) will always be true

@tomkennedy513
Copy link
Collaborator

superseded by #1305

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.

4 participants