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.

Sign up for GitHub

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

Jump to bottom

Dockerfiles phase 1 (detect changes) #860

Merged
merged 61 commits into from
Jun 8, 2022
Merged

Dockerfiles phase 1 (detect changes) #860

merged 61 commits into from
Jun 8, 2022

Conversation

natalieparellano
Copy link
Member

@natalieparellano natalieparellano commented May 24, 2022 •
edited
Loading

Generate changes to follow

Merging this PR should be blocked on #459 and #460 (we can update the platform api version in ##861)

The diff is quite huge due to the GroupBuildpack -> GroupElement rename. The most interesting files are probably:

  • cmd/lifecycle/detector.go
  • detector.go
  • detector_test.go
  • buildpack/detect.go
  • buildpack.detect_test.go

I based this branch off of cmd-package (#805) before it was merged, so there are several commits that don't actually relate to the detector, but they don't show up in the diff now that those commits are on main. We squash on merge so it shouldn't matter.

@natalieparellano
Acceptance tests pass with creator commented out
5a4832b 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Fix creator
ed1b3a7 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Add unit tests for analyze inputs
fc4a7e3 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Add scaffold for analyzer builder unit tests
2b505c6 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Add many unit tests, still have some TODOs
6fef994 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Address most TODOs
043974b 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Acceptance tests pass
02a5c74 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
A few more TODOs
785d81a 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Bring back go 1.16
e58d007 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Add unit test
125f01f 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Fix acceptance tests
4b4e816 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Fix units
8294ea5 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Remove nolint
fd6beb3 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Remove unneeded things
8335a56 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Use operations pattern
9ee1dfd 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Refactor tests to take advantage of operations pattern
d21352f 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Add missing tests
d81cb6c 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Fix lint
d815707 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Analyzer factory assigns "nop" services by default
46a24ae 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Try to fix registry handler test
fe83502 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Move new package from cmd/lifecycle/platform to platform/inputs
f72848d 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Merge branch 'main' into cmd-package
a457244 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Updates per PR review
c828d03 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Test the platform instead of the exiter
e0e8d8a 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Combine cmd/lifecycle/platform and platform
0af8ab2 
With some small changes to avoid an import cycle, we can make a meaningful platform package

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Clean up constructors
fcac2e3 
Eventually only the platform package should switch on platform api

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Remove ForAnalyzer struct
59097ca 
Having the analyzer factory take a list of args will ensure we don't forget
to update the creator when things change.

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Combine cmd/launcher/platform and platform
76811ce 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Remove comment
2d4a9bc 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Remove spec alias
e06cb9c 
Signed-off-by: Natalie Arellano <[email protected]>
This was referenced May 25, 2022
Open
Open
@natalieparellano
Decouple config handler and api verifier
ff567c8 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Move var to file where it is used
d070f0a 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Add tests
38287b8 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano natalieparellano marked this pull request as ready for review June 3, 2022 14:24
@natalieparellano natalieparellano requested a review from a team as a code owner June 3, 2022 14:24
mboldt
mboldt reviewed Jun 6, 2022
View reviewed changes
Copy link
Contributor

@mboldt mboldt left a comment

Choose a reason for hiding this comment

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

Overall looks good! Left some thoughts and questions (about stuff you've probably thought about a lot more than me, so take with a grain of salt ;-) ).

cmd/api_verifier.go Outdated Show resolved Hide resolved
buildpack/descriptor.go Outdated Show resolved Hide resolved
cmd/lifecycle/creator.go Show resolved Hide resolved
detector_test.go Show resolved Hide resolved
internal/lifecycle/utils.go Outdated Show resolved Hide resolved
@natalieparellano
Updates per PR review
e92a8b7 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Don't fail detect if /bin/detect and plan.toml are both missing
c518697 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Fix cherry pick
46375d0 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
APIVerifier takes a kind
3eb9961 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
DirStore takes a kind
3a0218d 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
detect_test.go uses kind
7130656 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Ensure that optional information is preserved
0a358b4 
- ReadOrder returns an order for extensions that has "extension = true" filled in
- Remove some duplication in the detector

Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Remove ToGroupElement() weirdness and fix comparison
bc0a646 
Signed-off-by: Natalie Arellano <[email protected]>
@natalieparellano
Copy link
Member Author

Thanks so much for your feedback @mboldt !

@natalieparellano natalieparellano merged commit 2e739d6 into main Jun 8, 2022
@natalieparellano natalieparellano deleted the dockerfiles-phase-1-detect branch June 8, 2022 14:09
This was referenced Jun 15, 2022
Merged
Merged
@natalieparellano natalieparellano self-assigned this Jun 16, 2022
@natalieparellano natalieparellano mentioned this pull request Jul 8, 2022
Merged
@natalieparellano natalieparellano mentioned this pull request Aug 23, 2022
Closed
@natalieparellano natalieparellano mentioned this pull request Nov 3, 2022
Open
@natalieparellano natalieparellano mentioned this pull request Feb 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mboldt mboldt mboldt approved these changes

Assignees

@natalieparellano natalieparellano

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@natalieparellano @mboldt