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

Pack detect #2132

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Pack detect #2132

wants to merge 4 commits into from

Conversation

rashadism
Copy link

Summary

Implementation for pack detect command. Up for feedback

Output

Before

After

Documentation

Runs only the analyze and detect phases and gives the selected group as output.

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

Related

Corresponding RFC

Resolves #___

@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Apr 18, 2024
@github-actions github-actions bot added this to the 0.34.0 milestone Apr 18, 2024
@jjbustamante jjbustamante modified the milestones: 0.34.0, 0.35.0 Apr 18, 2024
@rashadism
Copy link
Author

@natalieparellano @jjbustamante can you have a look

@github-actions github-actions bot added this to the 0.34.0 milestone Apr 19, 2024
@rashadism rashadism marked this pull request as ready for review April 19, 2024 05:28
@rashadism rashadism requested review from a team as code owners April 19, 2024 05:28
pkg/client/build.go Outdated Show resolved Hide resolved
@rashadism rashadism force-pushed the Pack-detect branch 5 times, most recently from 3fe2e35 to 841279f Compare April 22, 2024 07:49
@jjbustamante jjbustamante removed this from the 0.34.0 milestone Apr 22, 2024
Copy link
Member

@natalieparellano natalieparellano left a 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 Show resolved Hide resolved
internal/commands/detect.go Outdated Show resolved Hide resolved
internal/commands/detect.go Outdated Show resolved Hide resolved
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" +
Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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?

Copy link
Author

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

internal/commands/detect.go Outdated Show resolved Hide resolved
internal/build/lifecycle_execution.go Outdated Show resolved Hide resolved
internal/build/lifecycle_executor.go Show resolved Hide resolved
@github-actions github-actions bot added this to the 0.34.0 milestone May 7, 2024
@rashadism rashadism force-pushed the Pack-detect branch 4 times, most recently from 2d0b90b to d2ccb49 Compare May 8, 2024 06:18
@jjbustamante jjbustamante modified the milestones: 0.34.0, 0.35.0 May 8, 2024
@@ -40,6 +40,7 @@ type LifecycleExecution struct {
mountPaths mountPaths
opts LifecycleOptions
tmpDir string
// DetectOnly bool
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 need this?

Copy link
Author

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]>
@natalieparellano natalieparellano modified the milestones: 0.35.0, 0.36.0 Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants