Adding extension flag to summarize an extension buildpackage #344
Conversation
|
@mhdawson Can you have a look if everything looks right? |
There was a problem hiding this comment.
This looks good to me. There's one minor comment inline - about comments. It might be worth checking all the comments for buildpack and replacing them with extension - I didn't look too closely at this.
For "extra credit" you could consider what a solution to this problem might look like via generics. There's a lot of duplicated code throughout this codebase for the buildpack/extension combination, and we could simplify this back to one version of everything by using generics.
That being said, I'm certainly not going to block this PR on that idea!
internal/extension_inspector.go
Outdated
| } | ||
| defer layerGR.Close() | ||
|
|
||
| // Generally, each layer corresponds to a buildpack. |
There was a problem hiding this comment.
I'd update this comment to describe extensions, not buildpacks. Even though the words are the same :)
There was a problem hiding this comment.
Thanks for taking the time to have a look :) We discussed using a more generic implementation to avoid duplicate code, I'm also a big fan of this idea. However, with extensions, we are still determining some use cases that buildpacks support on whether extensions should supported them too. E.g. on the code you have commented, the question is, can an extension have multiple extensions, similar to what buildpacks do? It is possible but we don't have an actual use case that this would be necessary or would make sense. So, having a generic implementation when there are several grey areas will end up with a messy/hacky code. Therefore, we ended up keeping things separate till all scenarios around extensions compared to buildpacks have been covered.
There was a problem hiding this comment.
@robdimsdale renamed the variable and also rebased the branch to main with force push
pacostas
force-pushed
the
extension-summarize
branch
from
03a19be to
fefda1e
Compare
|
@robdimsdale Can you LGTM? |
|
@robdimsdale could you take another look. |
Summary
Adding extension command for summarizing a buildpackage.
Use Cases
This is used when we want to print info of an extension in a json or markdown format. Mostly used on the release notes of an extension.
Checklist