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

RFC: Group additions to Builder order #129

Merged
merged 6 commits into from
Mar 8, 2021
Merged

RFC: Group additions to Builder order #129

merged 6 commits into from
Mar 8, 2021

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Dec 23, 2020

@jkutner jkutner requested a review from a team as a code owner December 23, 2020 17:44
@jkutner jkutner force-pushed the group-additions branch 4 times, most recently from 6f02718 to 178e1f7 Compare December 23, 2020 17:46
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
id = "<buildpack ID>"
```

With the `pack` CLI, the `pre` and `post` buildpacks can be added with the `--pre-buildpack` and `--post-buildack` options.
Copy link
Member

Choose a reason for hiding this comment

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

Do we already use the terminology pre or post anywhere? What do you think of start/end or first/last?


The `after` and `before` keys define a requisite buildpack, which the injected buildpack will run either after or before accordingly.

If injected buildpacks need to conditionally run after more than one buildpack, the user can put the `after` and `before` keys in a `[[build.buildpacks.or]]` array of tables.
Copy link
Member

Choose a reason for hiding this comment

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

Can you put an example here? I wonder if we should scope this to simple use cases if we can't express it in the CLI.

text/0000-group-additions.md Outdated Show resolved Hide resolved
@hone
Copy link
Member

hone commented Jan 13, 2021

I assume that if you can't satisfy or the buildpack id doesn't exist it will fail?

@nebhale nebhale requested a review from a team January 13, 2021 20:33
@jromero
Copy link
Member

jromero commented Jan 14, 2021

If I recall correctly, there were some changes suggested for this RFC.

  1. The removal of a before concept since there was no concrete example IIRC.
  2. Changing after to something like depends-on essentially creating a dependency model.

Additional note: suggestions to move this to buildpack.toml were questioned given that package.toml already has an existing dependency model.

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

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

Would this mean deprecating from=builder?

As a side point, the motivation described doesn't really make it clear for me what's wrong with from=builder (other than it being undocumented), and how this would help. If you could clarify that in the RFC, that would definitely help me understand it a bit more

text/0000-group-additions.md Outdated Show resolved Hide resolved
jkutner and others added 2 commits January 20, 2021 08:38
Signed-off-by: Joe Kutner <jpkutner@gmail.com>

Co-authored-by: David Freilich <dfreilich@vmware.com>
@jkutner
Copy link
Member Author

jkutner commented Jan 20, 2021

@dfreilich yea, goodbye from=builder. I'll make that more clear (we've already started to do that in the code though https://github.com/buildpacks/pack/blob/main/internal/buildpack/locator_type.go#L61-L66)

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jromero
Copy link
Member

jromero commented Jan 20, 2021

Based on the changes made to this RFC, it seems like the scope can be minimized to only prepend and append buildpacks. If so, could this not already be done provided buildpack URIs?

As an example

...in project.toml:

[[build.buildpacks]]
url = "./my-pre-buildpack"

[[build.buildpacks]]
url = "urn:cnb:builder"

[[build.buildpacks]]
url = "./my-post-buildpack"

... in pack:

# pre
pack my-image -B my-builder -b ./my-buildpack/ -b urn:cnb:builder

# post
pack my-image -B my-builder -b urn:cnb:builder  -b ./my-buildpack/

@nebhale nebhale requested a review from a team January 20, 2021 21:44
@jkutner
Copy link
Member Author

jkutner commented Jan 21, 2021

@jromero i think we should kill urn:cnb:builder too, but that's definitely an alternative.

@jromero
Copy link
Member

jromero commented Jan 21, 2021

@jromero i think we should kill urn:cnb:builder too, but that's definitely an alternative.

If it's due to the fact that it's not intuitive I agree. Otherwise, I think it serves its purpose.

I have some slight reservations about boxing ourselves to pre and post terminology instead of a more robust solution but not strong enough to stand in the way of this RFC.

Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

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

Overall, I think that the change is net positive. I have some nits (to @hone's point, I'm not sure whether pre/post is more or less intuitive than start/end), but those are implementation details, and I'd rather discuss them on the PR to add this, then slow down the process here.

To my mind, while we do have a perfectly good way of doing it now (either --buildpack from=builder or --buildpack urn:cnb:builder), those aren't intuitive (even if they were documented), and having an explicit flag for users will definitely help them appreciate how they can adjust builders for their needs.

1 question: do you think that users should be able to specify multiple --pre or --post buildpacks? I think that it should be fine, and there's no reason to restrict them to just one, but I'd like to see that explicitly discussed.

@jkutner
Copy link
Member Author

jkutner commented Jan 21, 2021

@jromero yea, i actually tend to favor things don't require new flag or new things to learn, but I have two concerns about from=builder/urn:cnb:builder:

  • It's not intuitive
  • They actually represent a set of buildpacks (not a single buildpack) which makes it an odd-fellow with other single buildpacks defined with the --buildpack flag.

I have some slight reservations about boxing ourselves to pre and post terminology instead of a more robust solution but not strong enough to stand in the way of this RFC.

@jromero I do share this concern, which is why I was hoping to capture the before/after stuff too (but I think omitting it is the right thing to do now). My hunch though, is that changes to the middle of a default order/group will use a distinct mechanism regardless (especially since overriding a buildpack in the group will be necessary). All that said, I feel comfortable that the pre/post solution will ultimately not interfere with any future solution, and actually is solving a different kind of problem.

1 question: do you think that users should be able to specify multiple --pre or --post buildpacks? I think that it should be fine, and there's no reason to restrict them to just one, but I'd like to see that explicitly discussed.

@dfreilich yea, I think they would work like the normal --buildpack option (in terms of being a list)

@jkutner
Copy link
Member Author

jkutner commented Feb 3, 2021

@ekcasey @sclevine @nebhale PTAL

@sclevine
Copy link
Member

FCP starting 2/10

@sclevine
Copy link
Member

@ekcasey can you link the created issues?

@ekcasey
Copy link
Member

ekcasey commented Feb 18, 2021

@sclevine implementation is a noop. But sorry, I faked you out. I was going through this too quickly and forgot the project.toml is specified so core is not a noop. Creating the issue now.

@sclevine
Copy link
Member

@jromero @jkutner do we need to create any platform / learning issues?

@jkutner
Copy link
Member Author

jkutner commented Feb 22, 2021

@sclevine probably. at a minimum the project.toml docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants