Move logic out of cmd/lifecycle/analyzer.go

[natalieparellano]

Resolves #635. By moving logic out of main, we can simplify our acceptance tests. It should also make the logic easier to follow (especially with respect to platform api branching).

This still needs a few unit tests (marked with TODO) but I am marking it ready so as to receive feedback on the general direction.

If we like this, I will follow up with another PR to remove a bunch of tests from acceptance/analyzer_test.go.

I am hoping we could use this as a "template" for updating the other phases.

[jabrown85]

I didn't do a full review of every line or anything, but I 👍 the direction.

[natalieparellano]

@jabrown85 I think this is ready for another review. My only question at this point - what should be the home of github.com/buildpacks/lifecycle/cmd/lifecycle/platform? I was thinking github.com/buildpacks/lifecycle/platform, but I don't want to increase the dependencies of the launcher, which currently uses this package. What about github.com/buildpacks/lifecycle/platform/build and github.com/buildpacks/lifecycle/platform/launch?

[jabrown85]

This looks pretty good to me! It is a lot of changes. The tests are happy and I think it reads and feels better.

As far as the final package location goes, I think I like github.com/buildpacks/lifecycle/platform and github.com/buildpacks/lifecycle/platform/launch for the launcher specific deps. I think the naming is good for the one we use most of time and having a specific /launch package for the launch-specific things feels good. I am also happy to discuss alternatives on naming

[natalieparellano]

@jabrown85 I took a look at this today... the problem with putting everything that's currently in github.com/buildpacks/lifecycle/cmd/lifecycle/platform in github.com/buildpacks/lifecycle/platform is that it creates an import cycle:

package command-line-arguments
        imports github.com/buildpacks/lifecycle
        imports github.com/buildpacks/lifecycle/internal/layer
        imports github.com/buildpacks/lifecycle/platform    <---- things like platform.LayersMetadata
        imports github.com/buildpacks/lifecycle: import cycle not allowed

It also feels weird putting things like AnalyzeInputsResolver and LayersMetadata in the same package, because they have totally different consumers.

What about github.com/buildpacks/lifecycle/platform/cmd and github.com/buildpacks/lifecycle/platform/launch/cmd? Or something else?

Edit: I copy/pasted wrong before... fixed it ^^

[jabrown85]

What about github.com/buildpacks/lifecycle/cmd/lifecycle/platform/cmd and github.com/buildpacks/lifecycle/cmd/lifecycle/platform/launch/cmd? Or something else?

Would github.com/buildpacks/lifecycle/cli and github.com/buildpacks/lifecycle/cli/launch or github.com/buildpacks/lifecycle/cli/platform/github.com/buildpacks/lifecycle/cli/launch work?

If not, I'm also ok with github.com/buildpacks/lifecycle/cmd/lifecycle/platform/cmd, but don't really like the duplicate cmd in the same import. It feels bleh.

[natalieparellano]

These could maybe be made private

[natalieparellano]

Ah - I made them public so they could be tested here:

lifecycle/analyzer_test.go

Lines 104 to 105 in 8b9e787

	h.AssertEq(t, sbomRestorer.LayersDir, "some-layers-dir")
	h.AssertEq(t, sbomRestorer.Logger, logger)

[natalieparellano]

In the past, adding new inputs to the analyzer/restorer/etc. was a source of bugs because we would forget to update the creator. Accepting the inputs as a list, while ugly, can avoid this problem.

[natalieparellano]

I wanted this to be in the lifecycle package, but its dependence on verifyBuildpackApis (and needing to return the correct error code) made things a bit complicated.

[natalieparellano]

Hopefully, eventually! (for now just analyze...)

[natalieparellano]

These functions should eventually go somewhere else... but I think their new homes will be created when we do additional cleanup as part of buildpacks/rfcs#217

[mboldt]

Chatted with @natalieparellano - looks good!