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.

Sign up for GitHub

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

Jump to bottom

RFC-0078 Support for --pre-buildpack and --post-buildpack #1621

Merged
merged 7 commits into from
Feb 4, 2023
Merged

RFC-0078 Support for --pre-buildpack and --post-buildpack #1621

merged 7 commits into from
Feb 4, 2023

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Feb 3, 2023 •
edited

Summary

Implements RFC-0078

Fixes #1099

Output

Before

N/A

After

$ pack build --pre-buildpack jkutner/null@1.1.0 myapp

or

$ pack build --post-buildpack jkutner/null@1.1.0 myapp

or

[[io.buildpacks.pre.group]]
id = "jkutner/null"
version = "1.1.0"

[[io.buildpacks.post.group]]
uri = "docker://ghcr.io/jkutner/buildpacks/jkutner_null"

Documentation

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

Related

Resolves #___

@github-actions github-actions bot added this to the 0.29.0 milestone Feb 3, 2023
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Feb 3, 2023
@natalieparellano natalieparellano mentioned this pull request Feb 3, 2023
Open
6 tasks
@jkutner
RFC-0078 Support for --pre-buildpack and --post-buildpack
d67ab0a 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner jkutner marked this pull request as ready for review February 3, 2023 17:05
@jkutner jkutner requested review from a team as code owners February 3, 2023 17:05
joe-kimmel-vmw
joe-kimmel-vmw reviewed Feb 3, 2023
View reviewed changes
internal/commands/build.go Outdated Show resolved Hide resolved
pkg/client/build.go
if (len(order) == 0 || len(order[0].Group) == 0) && len(builderOrder) > 0 {
preBuildpacks := opts.PreBuildpacks
postBuildpacks := opts.PostBuildpacks
if len(preBuildpacks) == 0 && len(opts.ProjectDescriptor.Build.Pre.Buildpacks) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm almost surely just missing something but it's not obvious to me why the len(preBuildpacks)==0 check is important -- we wouldn't want to copy in the buildpacks from the projectdescriptor regardless?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, that's a good question... if we should merge --pre-buildpacks with opts.ProjectDescriptor.Build.Pre.Buildpacks. Or have it override (as it's written now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm leaning toward override because i'm not sure it's obvious which would come first in the order

Copy link
Member

Choose a reason for hiding this comment

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

I think merging stuff gets messy, so I'm 👍 on overriding.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool cool, thanks :)

@jkutner
Update internal/commands/build.go
5cbc5fe 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
jkutner
jkutner commented Feb 3, 2023
View reviewed changes
internal/commands/build.go Outdated Show resolved Hide resolved
@jkutner
Update internal/commands/build.go
d8a3962 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner
Update internal/builder/writer/human_readable.go
b28ae59 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@codecov
Copy link

codecov bot commented Feb 3, 2023 •
edited

Codecov Report

Merging #1621 (a16b3c6) into main (21d769e) will increase coverage by 0.01%.
The diff coverage is 79.47%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1621      +/-   ##
==========================================
+ Coverage   79.56%   79.56%   +0.01%     
==========================================
  Files         163      163              
  Lines       10661    10735      +74     
==========================================
+ Hits         8481     8540      +59     
- Misses       1668     1678      +10     
- Partials      512      517       +5
Flag Coverage Δ
os_linux 79.32% <79.47%> (+0.05%) ⬆️
os_macos 77.40% <79.47%> (-0.01%) ⬇️
os_windows 79.47% <79.47%> (+0.03%) ⬆️
unit 79.56% <79.47%> (+0.01%) ⬆️

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

@jkutner
Revert doc changes from refactor
04fb5a7 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner jkutner merged commit 2e05ee8 into main Feb 4, 2023
@jkutner jkutner deleted the jkutner/rfc-0078 branch February 4, 2023 20:13
@jjbustamante jjbustamante mentioned this pull request Aug 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-kimmel-vmw joe-kimmel-vmw joe-kimmel-vmw left review comments

@hone hone hone approved these changes

Assignees
No one assigned
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Milestone
0.29.0
Development

Successfully merging this pull request may close these issues.

RFC: Support for --pre-buildpack and --post-buildpack
3 participants
@jkutner @hone @joe-kimmel-vmw