-
Notifications
You must be signed in to change notification settings - Fork 294
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
Add validation support for empty/nil [[stacks]]
#2081
Add validation support for empty/nil [[stacks]]
#2081
Conversation
Signed-off-by: Josh W Lewis <josh.lewis@salesforce.com>
Signed-off-by: Josh W Lewis <josh.lewis@salesforce.com>
Signed-off-by: Josh W Lewis <josh.lewis@salesforce.com>
e6dae94
to
7a5d93b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2081 +/- ##
==========================================
+ Coverage 79.68% 79.69% +0.02%
==========================================
Files 176 176
Lines 13246 13254 +8
==========================================
+ Hits 10554 10562 +8
Misses 2022 2022
Partials 670 670
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -330,13 +330,22 @@ func (b *PackageBuilder) validate() error { | |||
|
|||
func (b *PackageBuilder) resolvedStacks() []dist.Stack { | |||
stacks := b.buildpack.Descriptor().Stacks() | |||
if len(stacks) == 0 && len(b.buildpack.Descriptor().Order()) == 0 { | |||
// For non-meta-buildpacks using targets, not stacks: assume any stack | |||
stacks = append(stacks, dist.Stack{ID: "*"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we need to add b.buildpack.Descriptor().API().AtLeast("0.10")
to the condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to do so if y'all want; just let me know. But my opinion is no. Old versions of the Buildpack API spec do not say that [[stacks]]
is required or mandatory. It seems like [[stacks]]
has been mandatory in pack
because of this logic, and perhaps that was unintentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am going to ask for @jkutner, @natalieparellano, and @hone opinions.
I searched in the git history and I can see that having a stack is mandatory, but I also agree that I didn't find it in the spec.
I would add the condition b.buildpack.Descriptor().API().AtLeast("0.10")
but I am not 100% sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we need to add b.buildpack.Descriptor().API().AtLeast("0.10") to the condition?
I would be okay leaving this out. Older buildpacks should be able to work on newer platforms (where stacks are not required) without having to make changes.
I agree that it seems the validation was enforcing the use of |
Fixed by buildpacks/pack#2081 and released in https://github.com/buildpacks/pack/releases/tag/v0.34.0 but not in a Buildpacks GHAs release at https://github.com/buildpacks/github-actions/releases yet
…or (#114) Fixed by buildpacks/pack#2081 and released in https://github.com/buildpacks/pack/releases/tag/v0.34.0 but not in a Buildpacks GHAs release at https://github.com/buildpacks/github-actions/releases yet
Summary
A
buildpack.toml
without[[order]]
and[[stacks]]
tables fails to pass validation duringpack buildpack package
as mentioned in #2047.[[stacks]]
was deprecated in API0.10
. Based on my experience with many other software deprecations, deprecations are usually hints to stop using that feature. It's surprising to me that pack would ask to specify[[stacks]]
while supporting a Buildpack API that marks[[stacks]]
as deprecated.This PR adds a new test (which fails on the main branch) that illustrates the problem. It also changes the validation a bit so that
pack
supports non-meta buildpacks without this table.This change will add support for empty stacks for Buildpack API versions prior to
0.10
. I believe this makes sense. Previous versions of the Buildpack API did not mark[[stacks]]
as mandatory, though it looks like this validation may have been accidentally doing so forpack
.Output
Before
After
Documentation
Related
Resolves #2047