-
Notifications
You must be signed in to change notification settings - Fork 288
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
Pack detect #2132
base: main
Are you sure you want to change the base?
Pack detect #2132
Conversation
@natalieparellano @jjbustamante can you have a look |
3fe2e35
to
841279f
Compare
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.
@rashadism thank you for taking the lead on this one. The implementation looks like it's progressing in the right direction. I left a few comments. I think we will want to add more test coverage (and at least one acceptance test although we can help you with that as the acceptance suite is notoriously tricky) but in general this looks good.
internal/commands/detect.go
Outdated
Args: cobra.ExactArgs(0), | ||
Short: "Run the detect phase of buildpacks against your source code", | ||
Example: "pack detect --path apps/test-app --builder cnbs/sample-builder:bionic", | ||
Long: "Pack Detect uses Cloud Native Buildpacks to run the detect phase of buildpack groups against the source code.\n" + |
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.
Maybe this part could describe how/where to expect the output of running pack detect
.
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.
where the output is shown wasn't touched on specifically in the RFC. We can either output to stdout or write it to disk. What do you think?
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.
That's a good question - I think since we'd want both the log output from each buildpack's ./bin/detect as well as the resulting group.toml and plan.toml, writing to disk makes the most sense. pack build
has --report-output-dir
and --sbom-output-dir
configurable options, maybe we make a --detect-output-dir
?
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.
Yes, I included the --detect-output-dir
in the corresponding RFC. So that will be the way to go
2d0b90b
to
d2ccb49
Compare
@@ -40,6 +40,7 @@ type LifecycleExecution struct { | |||
mountPaths mountPaths | |||
opts LifecycleOptions | |||
tmpDir string | |||
// DetectOnly bool |
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.
Do we need this?
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 don't think so. There are some conflicts as well. WIll tidy it up and and go through everything again and push
Signed-off-by: Rashad Sirajudeen <[email protected]>
Signed-off-by: Rashad Sirajudeen <[email protected]>
Signed-off-by: Rashad Sirajudeen <[email protected]>
Signed-off-by: Rashad Sirajudeen <[email protected]>
Summary
Implementation for
pack detect
command. Up for feedbackOutput
Before
After
Documentation
Runs only the analyze and detect phases and gives the selected group as output.
Related
Corresponding RFC
Resolves #___