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 for pack detect #308

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

RFC for pack detect #308

wants to merge 9 commits into from

Conversation

rashadism
Copy link

A basic implementation on pack detect.
Looking forward on getting feedback and improving on this and coming up with the best version of this, and implenenting this.

@buildpack-bot
Copy link
Member

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

rashadism and others added 2 commits February 18, 2024 22:01
Signed-off-by: Rashad Sirajudeen <rashad.20@cse.mrt.ac.lk>
Signed-off-by: Rashad Sirajudeen <rashad.20@cse.mrt.ac.lk>
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.

Thanks for kicking this off @rashadism! I would love to see this happen :)

@rashadism
Copy link
Author

Thank you @natalieparellano , will update on the stuff you pointed out.

Signed-off-by: Rashad Sirajudeen <rashad.20@cse.mrt.ac.lk>
@natalieparellano natalieparellano changed the title Initial draft, up for feedback RFC for pack detect Mar 27, 2024
@rashadism rashadism mentioned this pull request Apr 18, 2024
2 tasks
@rashadism
Copy link
Author

I opened a corresponding draft PR . @natalieparellano can u have a look

@natalieparellano
Copy link
Member

@jjbustamante is going to steward this one :)

@rashadism
Copy link
Author

@jjbustamante is going to steward this one :)

nice, looking forward to it (:

text/0000-my-feature.md Outdated Show resolved Hide resolved
- The main use case of `pack build` is to create OCI images, and detect is just a binary in the lifecycle, so it doesn't make much sense to include it in there.
- To avoid making the mostly used `pack build` command overly complicated.

Also, instead of `pack detect`, something like `pack execute --phase detect` can also be done, where the user can select exclusively what phase they need to run. Can start by implementing just the `detect` phase.
Copy link

@dmikusa dmikusa May 13, 2024

Choose a reason for hiding this comment

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

This might be worth doing. It could be nice for integration testing purposes to be able to run just the build or just the generate phases of a buildpack. Not that we'd include those into this RFC, but this would leave the CLI open for that possibility.

When it comes to integration testing buildpacks with complicated build plans, right now, you have two choices:

  1. Don't integration test them.
  2. Supply all the buildpacks required to run the buildpack under test. That way all the set up and requirements for this buildpack are complete.

If we could run build or generate specifically, then that would open up the ability to mock out the requirements and run more focused, faster integration tests.

Anyway, that is all just to say I would be in support of this CLI invocation as it leaves open the possibility of running other phases directly.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it looks like the better path to take

text/0000-my-feature.md Outdated Show resolved Hide resolved
||--run-image|string|
||--uid|int|
||--workspace|string|

Copy link

Choose a reason for hiding this comment

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

What would be the plan for the exit code of this command?

I think we should specify this because it's likely that we'll build tooling around this to run tests. We want the exit code to signal errors and for it to be stable.

Copy link
Author

Choose a reason for hiding this comment

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

If I understood your question correctly,
Ideally it should be 0 if a build plan selection is successful. In the case of failure, we can come up with some common cases and define it accordingly.

Copy link

Choose a reason for hiding this comment

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

Ok, yes. 0 for success and 1 for failure probably covers most of what is necessary. I'm not sure we need to define a bunch of non-zero exit codes. It does make me wonder what the lifecycle returns 🤔 Maybe we should just make sure the lifecycle exit code is returned from this command? I suspect we'd get good results that way and then this command would be inline with the platform spec.

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 we also need to add some examples executions of the command with the expected output, that helps a little bit to understand better what is going to do and if we need to pay attention to some details.

rashadism and others added 3 commits May 14, 2024 10:03
Co-authored-by: Daniel Mikusa <dan@mikusa.com>
Signed-off-by: Rashad Sirajudeen <rashadsirajudeen@gmail.com>
Signed-off-by: Rashad Sirajudeen <rashadsirajudeen@gmail.com>
Signed-off-by: Rashad Sirajudeen <rashadsirajudeen@gmail.com>
@natalieparellano
Copy link
Member

I think this one might be ready for another round of reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/team RFC scoped to a sub-team as opposed to the entire project. status/steward-assigned team/platform type/rfc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants