Fix pack buildpack new --targets

[natalieparellano]

Summary

Output

Before

After

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

Related

Resolves #2120
Resolves #2121

[codecov]

Codecov Report

Attention: Patch coverage is 85.41667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 70.21%. Comparing base (aef63b5) to head (e397773).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2123      +/-   ##
==========================================
+ Coverage   70.19%   70.21%   +0.02%     
==========================================
  Files         233      233              
  Lines       17086    17097      +11     
==========================================
+ Hits        11992    12003      +11     
  Misses       4325     4325              
  Partials      769      769              

Flag	Coverage Δ
os_linux	69.32% <85.42%> (+0.01%)	⬆️
os_macos	65.90% <85.42%> (+0.02%)	⬆️
os_windows	69.73% <85.42%> (+0.02%)	⬆️
unit	70.21% <85.42%> (+0.02%)	⬆️

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

[jjbustamante]

I am not sure how many people started using these new fields, but I will need to leave some Note in the release note for pack 0.34.0

[robdimsdale]

The paketo team (and Broadcom commercial teams) ran into an issue around this. Specifically, newer versions of the lifecycle require [[targets.distributions]] in buildpack.toml but pack build does not pick up those fields correctly.

[jjbustamante]

I ran the scenario from @edmorley

Note: pack is the binary compiled from the branch

>  pack  buildpack new testcase-targets-distros --api 0.10 --targets "linux/amd64:ubuntu@123"
    create  buildpack.toml
    create  bin/build
    create  bin/detect
Successfully created testcase-targets-distros
> cat testcase-targets-distros/buildpack.toml
api = "0.10"
WithWindowsBuild = false
WithLinuxBuild = false

[buildpack]
  id = "testcase-targets-distros"
  version = "1.0.0"

[[targets]]
  os = "linux"
  arch = "amd64"

  [[targets.distros]]
    name = "ubuntu"
    version = "123"

Everything looks fine

[jjbustamante]

I did the following test, I tried to package the Buildpack generated.

Using the branch binary everything is fine, as expected

> $PACK_BINARY_FROM_BRANCH buildpack package testcase-targets-distros --format file
Successfully created package testcase-targets-distros.cnb and saved to file

And then, I tried to use the current pack 0.33.2 version

> pack buildpack package testcase-targets-distros-2 --format file
ERROR: no compatible stacks among provided buildpacks

As expected an error must be thrown because we changed the toml schema, my only concern is, should we add a helpful message for end-users to point them out to update:

• distributions -> distros
• versions -> version (single value instead of an array)

This could happen if we try to execute the new pack version, containing this fix, with old toml schema

[jjbustamante]

Sorry about all the messages.

The new pack version containing this feature is not going to fail with a buildpack using the previous schema, so things could change for end-users and they wouldn't notice

These examples uses the binary for this branch and package the same buildpack with the previous and the new schema

# Metadata is fine, it has the expected values
> docker inspect targets-distros-new-schema-new-version  | jq '.[] | .Config.Labels | ."io.buildpacks.buildpack.layers" | fromjson' | jq
{
  "testcase-targets-distros": {
    "1.0.0": {
      "api": "0.10",
      "targets": [
        {
          "os": "linux",
          "arch": "amd64",
          "distros": [
            {
              "name": "ubuntu"
            }
          ]
        }
      ],
      "layerDiffID": "sha256:5274312d546cfc8a8803d2ba932df72a27c8e65fb5015f4a6870dfef030764bf"
    }
  }
}

# Metadata is missing distros/name but the package is created.
> docker inspect targets-distros-old-schema-new-version  | jq '.[] | .Config.Labels | ."io.buildpacks.buildpack.layers" | fromjson' | jq
{
  "testcase-targets-distros": {
    "1.0.0": {
      "api": "0.10",
      "targets": [
        {
          "os": "linux",
          "arch": "amd64"
        }
      ],
      "layerDiffID": "sha256:60a056247c8b9513c22737dbca36a092836a9cd4bbedb4cf64f7f2172938df21"
    }
  }
}

I think we should:

• Failed creating the buildpack package if we detect the distributions and versions
• Or at least throw an warning message to get end-users attention, I am not sure about this, because if they are using a CI pipeline and the command runs successfully they will not pay attention to the warning

[edmorley]

I think we should:

* Failed creating the buildpack package if we detect the `distributions` and `versions`

* Or at least throw an **warning** message to get end-users attention, I am not sure about this, because if they are using a CI pipeline and the command runs successfully they will not pay attention to the warning

I'd love to see a warning (or error) added if possible, since it would have allowed me to realise sooner that targets.distributions wasn't the correct table name (I'd been using targets.distributions since both the RFC and pack buildpack new used it).

However, I wonder if this would be best implemented as more strict buildpack.toml schema validation in general? ie: Warn (or error) on any unknown table/field names. Doing this would also catch cases like the version vs versions issue seen in buildpacks/spec#401 too.

[edmorley]

Fixes #2120

From skim reading, I think this PR might also fix #2121, thanks to the switch from if len(d) <= 2 to if len(d) < 2?

[jjbustamante]

From skim reading, I think this PR might also fix #2121, thanks to the switch from if len(d) <= 2 to if len(d) < 2?

Yeap, it is also fixing #2121

[natalieparellano]

I can update this PR to add a warning if we encounter wrong stuff in buildpack.toml

[natalieparellano]

@jjbustamante @edmorley I updated the PR to add the warnings. We should be good to go!

[edmorley]

Amazing - looks great, thank you!

[jjbustamante]

Awesome work @natalieparellano !!! ❤️