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

Stack removal: detect phase and analyze.toml support Targets #994

Merged
merged 21 commits into from
Mar 7, 2023

Conversation

joe-kimmel-vmw
Copy link
Contributor

@joe-kimmel-vmw joe-kimmel-vmw commented Jan 31, 2023

fixes: #743

  • removes stacks from buildpack descriptors and detection logic,
  • shims from stack to target where relevant.
  • changes and simplifies some pointer-struct to just strings, which does change the analyze.toml spec. As far as I can tell this is victimless change, but probably there's a corresponding PR in the specs repo I haven't made yet?

@joe-kimmel-vmw joe-kimmel-vmw force-pushed the 743-probably branch 4 times, most recently from ac9fb02 to 986d801 Compare February 6, 2023 22:28
@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #994 (7b5e9f3) into main (014cceb) will increase coverage by 0.15%.
The diff coverage is 22.59%.

❗ Current head 7b5e9f3 differs from pull request most recent head daa0182. Consider uploading reports for the commit daa0182 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #994      +/-   ##
==========================================
+ Coverage   64.48%   64.62%   +0.15%     
==========================================
  Files          82       86       +4     
  Lines        6181     6231      +50     
==========================================
+ Hits         3985     4026      +41     
- Misses       1810     1824      +14     
+ Partials      386      381       -5     
Flag Coverage Δ
os_linux 64.08% <22.59%> (-0.39%) ⬇️
os_windows 59.00% <22.59%> (?)
unit 64.08% <22.59%> (-0.39%) ⬇️

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

@joe-kimmel-vmw joe-kimmel-vmw changed the title very wip, incomplete changes still wip but getting somewheres probably Feb 8, 2023
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.

@joe-kimmel-vmw it's awesome to see this taking shape. I left a few comments for now

buildpack/testdata/buildpack/by-id/B/v1/bin/build Outdated Show resolved Hide resolved
platform/files.go Outdated Show resolved Hide resolved
buildpack/bp_descriptor.go Outdated Show resolved Hide resolved
cmd/lifecycle/creator.go Outdated Show resolved Hide resolved
buildpack/bp_descriptor.go Outdated Show resolved Hide resolved
detector.go Outdated Show resolved Hide resolved
@joe-kimmel-vmw joe-kimmel-vmw force-pushed the 743-probably branch 4 times, most recently from edb21ff to 357fc37 Compare February 15, 2023 00:14
@joe-kimmel-vmw joe-kimmel-vmw changed the title still wip but getting somewheres probably Stack removal: little ol detect phase and the big bad analyzed.toml Feb 16, 2023
@joe-kimmel-vmw joe-kimmel-vmw marked this pull request as ready for review February 17, 2023 00:15
@joe-kimmel-vmw joe-kimmel-vmw requested a review from a team as a code owner February 17, 2023 00:15
platform/files.go Outdated Show resolved Hide resolved
platform/export_inputs.go Outdated Show resolved Hide resolved
platform/files.go Outdated Show resolved Hide resolved
detector.go Outdated Show resolved Hide resolved
cmd/lifecycle/restorer.go Outdated Show resolved Hide resolved
detector.go Outdated Show resolved Hide resolved
detector.go Outdated Show resolved Hide resolved
platform/files.go Outdated Show resolved Hide resolved
@joe-kimmel-vmw joe-kimmel-vmw changed the title Stack removal: little ol detect phase and the big bad analyzed.toml Stack removal: detect phase and analyze.toml support Targets Mar 2, 2023
@joe-kimmel-vmw joe-kimmel-vmw force-pushed the 743-probably branch 3 times, most recently from c260907 to f30221d Compare March 2, 2023 03:31
BpDescriptor reads and populates Targets data

Signed-off-by: Joe Kimmel <[email protected]>
minor cleanup
nix mixins mixup

Signed-off-by: Joe Kimmel <[email protected]>
- RunImage has:
  - Reference
  - Target
While i was in there, some of the other *ImageReference types were
inconvient so I changed them to just be strings instead of structs
containing strings. This does have the effect of collapsing a table,
But as a newer contributor to this project I have the blessing and curse
of not feeling all that constrained by (nor aware of) past decisions.

Signed-off-by: Joe Kimmel <[email protected]>
@joe-kimmel-vmw joe-kimmel-vmw force-pushed the 743-probably branch 2 times, most recently from bc6c9de to 81ff6ca Compare March 3, 2023 20:26
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.

Adding a few comments for now, still reviewing

env/build.go Outdated Show resolved Hide resolved
buildpack/bp_descriptor_test.go Outdated Show resolved Hide resolved
buildpack/bp_descriptor.go Outdated Show resolved Hide resolved
buildpack/bp_descriptor.go Outdated Show resolved Hide resolved
handlers.go Outdated Show resolved Hide resolved
platform/files.go Outdated Show resolved Hide resolved
joe-kimmel-vmw and others added 6 commits March 6, 2023 20:29
Update env/build.go
Update buildpack/bp_descriptor.go
Update default arch to amd64
use OS instead of Os
fixing mocks and fn calls after small change

Co-authored-by: Natalie Arellano <[email protected]>
Signed-off-by: Joe Kimmel <[email protected]>
fix descriptor test bc we decided that we _do_ want that behavior for old versions

Signed-off-by: Joe Kimmel <[email protected]>
platform/files.go Outdated Show resolved Hide resolved
Joe Kimmel and others added 3 commits March 7, 2023 13:40
This allows us to remove our custom writer, while keeping the file schema unchanged in Platform API 0.12

Signed-off-by: Natalie Arellano <[email protected]>
Signed-off-by: Joe Kimmel <[email protected]>
Signed-off-by: Natalie Arellano <[email protected]>
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.

Looks good to me! Thanks for all your hard work @joe-kimmel-vmw

@natalieparellano natalieparellano merged commit 6d6b469 into buildpacks:main Mar 7, 2023
@natalieparellano natalieparellano deleted the 743-probably branch March 7, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC #0096] Lifecycle should provide CNB_TARGET_ID in buildpack env
3 participants