added targets flag for buildpack new cli #1921

WYGIN · 2023-09-25T12:26:55Z

Summary

added new flag --targets to pack buildpack new with shorthand -t
now this new flag --targets will take args in format [os][/arch][/archvariant]:[distroName@version@another-version];[another-distro@v1@v2]
mark --stacks flag deprecated
removed the default value of stacks, so id of stacks no more exists in file buildpack.toml
remove [[stacks]] from buildpack.toml file
will add a default --target of current device when target flag is not specified
will show an error on unknown target is added from cli (only for [os] [arch] [archVariant]) but still adds those targets to final output of buildpack.toml file

Output

Before

pack buildpack new test

api = "0.8"
WithWindowsBuild = false
WithLinuxBuild = false

[buildpack]
  id = "test"
  version = "1.0.0"

[[stacks]]
  id = "io.buildpacks.stacks.jammy"

After

pack buildpack new test -t "linux/arm/v6" -t "linux/amd64" -t "windows/amd64:[email protected]" -t "linux/arm/v6:[email protected]" -t "linux/arm/v6:[email protected]" -t "linux/arm/v6:[email protected]@16.02;[email protected]@8.06"

api = "0.8"
WithWindowsBuild = false
WithLinuxBuild = false

[buildpack]
  id = "test"
  version = "1.0.0"

[[targets]]
  os = "linux"
  arch = "arm"
  variant = "v6"

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

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

  [[targets.distributions]]
    name = "windows-nano"
    versions = ["10.0.19041.1415"]

[[targets]]
  os = "linux"
  arch = "arm"
  variant = "v6"

  [[targets.distributions]]
    name = "ubuntu"
    versions = ["14.04"]

[[targets]]
  os = "linux"
  arch = "arm"
  variant = "v6"

  [[targets.distributions]]
    name = "ubuntu"
    versions = ["16.04"]

[[targets]]
  os = "linux"
  arch = "arm"
  variant = "v6"

  [[targets.distributions]]
    name = "ubuntu"
    versions = ["16.01", "16.02"]

  [[targets.distributions]]
    name = "debian"
    versions = ["10.10", "8.06"]

Documentation

Should this change be documented?
- Yes, see # doc for --targets flag
- No

buildpack_new.go:112: Unexpected call to *testmocks.MockPackClient.NewBuildpack([context.Background {0.8 /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000g
n/T/build-test163248382/some-cnb example/some-cnb 1.0.0 [] [{   []}]}]) 
at /Users/runner/work/pack/pack/internal/commands/buildpack_new.go:112 because:
expected call at /Users/runner/work/pack/pack/internal/commands/buildpack_new_test.go:58 doesn't match the argument at index 1.

Got: {0.8 /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/build-test163248382/some-cnb example/some-cnb 1.0.0 [] [{   []}]} (client.NewBuildpackO
ptions)

 Want: is equal to {0.8 /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/build-test163248382/some-cnb example/some-cnb 1.0.0 [{io.buildpacks.stacks
.jammy []}] [{linux arm v6 [{ubuntu [14.04 16.04]}]}]} (client.NewBuildpackOptions)

It seems to be because of the new argument, the Got expectation doesn't know about the new platform values. I think you may need to re-create the Mock.

jjbustamante · 2023-09-27T15:03:33Z

I think I know what is going on, if you take a look at your change in internal/commands/buildpack_new_test.go you said:

I am expecting the pack client to be called with these arguments, and you added the Targets stuff

But in the line 78

path := filepath.Join(tmpDir, "some-cnb")
command.SetArgs([]string{"--path", path, "example/some-cnb"})

Where you are actually setting the flags to the pack buildpack package command, you are not setting the --targets flag there. You should set the values you are expecting to process there.

A suggestion, do not modify the current tests just add a new test case for the new scenario.

when("BuildpackNew#Execute", func() {
	it("uses the args to generate artifacts", func() { 
              // This is the current existing test
        }   
})

You could modify it in this way:

when("BuildpackNew#Execute", func() {
	it("uses the args to generate artifacts", func() { 
              // This is the current existing test
        }   
        when("targets is specified", func() { 
              it("uses the targets to generate artifacts", func(){ 
                 // Copy here copy from the previous test, and modify it as you need. 
                 // Also any refactor is valid to avoid code duplication
              }
        }
})

Signed-off-by: WYGIN <[email protected]>

WYGIN · 2023-09-28T15:36:58Z

@jjbustamante @natalieparellano pr is ready for review, please let me know if there there is something to fix

internal/commands/buildpack_new.go

Signed-off-by: WYGIN <[email protected]>

jjbustamante · 2023-09-29T23:03:30Z

@WYGIN Amazing work!

I compiled your branch and played with it. I have a little doubt with one scenario, @natalieparellano maybe you can help me to validate the expected behavior.

I ran the example from our docs

>  out/pack buildpack new examples/ruby \
    --api 0.8 \
    --path ruby-buildpack \
    --version 0.0.1 \
    --stacks io.buildpacks.samples.stacks.jammy
Flag --stacks has been deprecated, prefer `--targets` instead: https://github.com/buildpacks/rfcs/blob/main/text/0096-remove-stacks-mixins.md
    create  buildpack.toml
    create  bin/build
    create  bin/detect
Successfully created examples/ruby

The structure was created, but checking the buildpack.toml

> cat ruby-buildpack/buildpack.toml
api = "0.8"
WithWindowsBuild = false
WithLinuxBuild = false

[buildpack]
  id = "examples/ruby"
  version = "0.0.1"

[[stacks]]
  id = "io.buildpacks.samples.stacks.jammy"

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

We have stacks and targets, and I think we can't have both

By the way, I checked the latest pack version 0.31.0 and the buildpack.toml is filled with the following content:

> cat ruby-buildpack/buildpack.toml
api = "0.8"
WithWindowsBuild = false.   
WithLinuxBuild = false

[buildpack]
  id = "examples/ruby"
  version = "0.0.1"

[[stacks]]
  id = "io.buildpacks.samples.stacks.jammy"

@jkutner Should WithWindowsBuild and WithLinuxBuild be writing down into the buildpack.toml? I didn't see those fields specify in the RFC and I am wondering if this is a bug, and those are just implementation details.

WYGIN · 2023-09-30T05:40:45Z

We have stacks and targets, and I think we can't have both

if i am not wrong it is part of build time but i think we can add a warning for pack buildpack new when both stacks and targets specified

@jkutner Should WithWindowsBuild and WithLinuxBuild be writing down into the buildpack.toml? I didn't see those fields specify in the RFC and I am wondering if this is a bug, and those are just implementation details.

i think WithWindowsBuild and WithLinuxBuild are used with stacks flag & having it part of buildpack.toml file might not be needed when --stacks flag is not used

i am also thinking about adding cli auto completion when defining targets but it might not be very useful if users only target few platforms, so i would love to add autocompletion if it improves UX of the user

Signed-off-by: WYGIN <[email protected]>

WYGIN · 2023-10-02T04:50:45Z

@jjbustamante i would like to know if there are any changes still required, i am willing to work on this issue(#1632 ) if i am done with this pr

jjbustamante · 2023-10-02T14:15:46Z

if i am not wrong it is part of build time but i think we can add a warning for pack buildpack new when both stacks and targets specified

Yes, the validation is executed during build time, but, what I am totally happy with is the following:

pack buildpack new command is meant to create a folder structure for a new buildpack, but, if we create a scaffolding that fails later with other command, it will confuse the end-user. I want to avoid creating a wrong structure. I am going to add a code suggestion for that case.

jjbustamante · 2023-10-02T14:17:05Z

internal/commands/buildpack_new_test.go

+							ID:     "io.buildpacks.stacks.jammy",
+							Mixins: []string{},
+						}},
+						Targets: targets,


Based on my comment here, I will remove this Targets: targets.

IF users specifies only stacks, I don't want targets to be added in buildpack.toml, the warning message is great and we will keep the current behavior of pack.

IF users specifies stacks and targets then both are allow for now, until stacks is actually deprecated. Based on this slack thread

internal/commands/buildpack_new.go

Signed-off-by: WYGIN <[email protected]>

jjbustamante

@WYGIN thank you so much for this PR!! ❤️ , it is going to be very valuable for our end goal for supporting multi-arch!

Great job!

jjbustamante · 2023-10-11T17:00:38Z

@buildpacks/platform-maintainers can you take a look at this? and merge if everything is fine.

added targets flag for buildpack new cli

d543d17

Signed-off-by: WYGIN <[email protected]>

github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Sep 25, 2023

github-actions bot added this to the 0.32.0 milestone Sep 25, 2023

added format to add --targets from cli

96b28c3

Signed-off-by: WYGIN <[email protected]>

jjbustamante reviewed Sep 26, 2023

View reviewed changes

internal/commands/buildpack_new.go Outdated Show resolved Hide resolved

WYGIN added 2 commits September 27, 2023 10:39

refactored code to receive flags from []string for --targets flag

057f53d

Signed-off-by: WYGIN <[email protected]>

fix: bug of distro not being added to toml file

16b8350

Signed-off-by: WYGIN <[email protected]>

WYGIN marked this pull request as ready for review September 27, 2023 12:04

WYGIN requested review from a team as code owners September 27, 2023 12:04

WYGIN added 3 commits September 28, 2023 14:47

added tests & show error msg when unknow target is passed

e79226e

Signed-off-by: WYGIN <[email protected]>

fixed tests

ca5d09f

Signed-off-by: WYGIN <[email protected]>

removed persistent flag

90c2195

Signed-off-by: WYGIN <[email protected]>

jjbustamante reviewed Sep 28, 2023

View reviewed changes

internal/commands/buildpack_new.go Outdated Show resolved Hide resolved

WYGIN added 2 commits September 29, 2023 17:48

refactored code & still need to fix bugs

210757c

Signed-off-by: WYGIN <[email protected]>

fix bugs

77dac7f

Signed-off-by: WYGIN <[email protected]>

WYGIN added 3 commits October 1, 2023 20:17

added warns to targets flag

184f6c3

Signed-off-by: WYGIN <[email protected]>

fix tests

9a5036c

Signed-off-by: WYGIN <[email protected]>

lint fix

e7902f8

Signed-off-by: WYGIN <[email protected]>

jjbustamante reviewed Oct 2, 2023

View reviewed changes

internal/commands/buildpack_new.go Outdated Show resolved Hide resolved

will not be added to when is defined and not defined

cdd9417

Signed-off-by: WYGIN <[email protected]>

jjbustamante approved these changes Oct 3, 2023

View reviewed changes

jjbustamante mentioned this pull request Oct 3, 2023

Suggest/trust heroku/builder:20 instead of heroku/buildpacks:20 #1924

Merged

jkutner approved these changes Oct 17, 2023

View reviewed changes

jkutner enabled auto-merge October 17, 2023 20:28

Merge branch 'main' into buildpack-new-targets-flag

d84b81f

jkutner merged commit a2b862c into buildpacks:main Oct 17, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added targets flag for buildpack new cli #1921

added targets flag for buildpack new cli #1921

WYGIN commented Sep 25, 2023 •

edited

Loading

WYGIN commented Sep 27, 2023 •

edited

Loading

jjbustamante commented Sep 27, 2023

jjbustamante commented Sep 27, 2023

jjbustamante commented Sep 27, 2023

WYGIN commented Sep 28, 2023

jjbustamante commented Sep 29, 2023 •

edited

Loading

WYGIN commented Sep 30, 2023 •

edited

Loading

WYGIN commented Oct 2, 2023

jjbustamante commented Oct 2, 2023

jjbustamante Oct 2, 2023 •

edited

Loading

jjbustamante left a comment

jjbustamante commented Oct 11, 2023

added targets flag for buildpack new cli #1921

added targets flag for buildpack new cli #1921

Conversation

WYGIN commented Sep 25, 2023 • edited Loading

Summary

Output

Before

After

Documentation

Related

WYGIN commented Sep 27, 2023 • edited Loading

jjbustamante commented Sep 27, 2023

jjbustamante commented Sep 27, 2023

jjbustamante commented Sep 27, 2023

WYGIN commented Sep 28, 2023

jjbustamante commented Sep 29, 2023 • edited Loading

WYGIN commented Sep 30, 2023 • edited Loading

WYGIN commented Oct 2, 2023

jjbustamante commented Oct 2, 2023

jjbustamante Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

jjbustamante left a comment

Choose a reason for hiding this comment

jjbustamante commented Oct 11, 2023

WYGIN commented Sep 25, 2023 •

edited

Loading

WYGIN commented Sep 27, 2023 •

edited

Loading

jjbustamante commented Sep 29, 2023 •

edited

Loading

WYGIN commented Sep 30, 2023 •

edited

Loading

jjbustamante Oct 2, 2023 •

edited

Loading