Lifecycle maintainability when supporting multiple api versions

[natalieparellano]

The lifecycle currently supports all buildpack APIs (currently, 0.2-0.6) and all platform APIs (currently, 0.3-0.7). Having a single binary that supports all buildpack APIs is important for end users - they should not have to worry about whether a set of buildpacks will work together. However having a single binary that supports all platform APIs might not be necessary.

Some possible ways forward that have been discussed:

• Have a separate binary (branch?) per platform
• Consolidate platform-specific logic in the platform package, and similarly consolidate buildpack-specific logic in the buildpack package
• Duplicate a bunch of code

To aid in moving this conversation forward, we thought it would be helpful to document & describe all of the ways that supporting multiple api versions is causing us pain. Then it might be clearer what are the next steps we should take.

Please add your thoughts!

[natalieparellano]

Code in cmd like this:

	if a.platformAPIVersionGreaterThan06() {
		cmd.FlagPreviousImage(&a.previousImageRef)
		cmd.FlagRunImage(&a.runImageRef)
		cmd.FlagStackPath(&a.stackPath)
		cmd.FlagTags(&a.additionalTags)
	} else {
		cmd.FlagCacheDir(&a.platform06.cacheDir)
		cmd.FlagGroupPath(&a.platform06.groupPath)
		cmd.FlagSkipLayers(&a.platform06.skipLayers)
	}

and

	if a.restoresLayerMetadata() {
		if a.cacheImageRef == "" && a.platform06.cacheDir == "" {
			cmd.DefaultLogger.Warn("Not restoring cached layer metadata, no cache flag specified.")
		}
	}

We should have one analyzeCmd per platform. Code like a.platform06.cacheDir is hard to reason about. And having branching logic depending on the platform makes Args() hard to read.

[jromero]

There are some common patterns that should help you at least think of possible solutions. Additionally, given that they can be recognized by other developers that know the patterns helps reason about them better.

• https://github.com/bvwells/go-patterns#design-patterns
• https://github.com/tmrts/go-patterns

[yaelharel]

Our acceptance tests are full of SkipIf. For example:

h.SkipIf(t,
              api.MustParse(platformAPI).LessThan("0.5") || api.MustParse(platformAPI).AtLeast("0.7"),
              "Platform API < 0.5 reads and writes to the working directory; Platform 0.7+ does not accept a -cache-dir flag",)

It's really hard to write and read such tests.

[jabrown85]

Adding on the acceptance tests and flags, we have a lot of logic around passing flags dynamically based on version.

				if api.MustParse(platformAPI).Compare(api.MustParse("0.7")) >= 0 {
					analyzeFlags = append(analyzeFlags, []string{"-run-image", fixtures.authRegRunImage}...)
				}

We also have "--env", "CNB_PLATFORM_API="+platformAPI, in almost every acceptance test. Acceptance tests also often rely on files that live in source (stack.toml, order.toml, etc.). These files have to either be duplicated or written at runtime in order to match what a platform version expects when we iterate on these files. Since we often share tests, this usually ends up being code similar to above or a duplication of the relevant tests themselves.

Even if we had unique analyzeCmd, we would still have this type of code in acceptance test. If we were to either use a branch per release or duplicate acceptance tests for each release we could eliminate this type of code. The duplication would be across boundaries (pkg or branch).

I've also been thinking about what it may look like to adjust paths and env conventions if the upcoming spec gets the green light. That sounds miserable to have to support both in every test.

Not related directly to the maintenance of the code, but I think it is also worth discussing how we can de-risk minor platform updates for platform operators. Today, if an operator wanted to get a bug fix for platform 0.6 that happens to be found near the release of vNext lifecycle, they would end up taking the vNext of lifecycle that may include a large change like reversing the order of analyzer and detector. We have loads of tests to protect the interface of 0.6, but I would still be concerned taking a new lifecycle binary that has such a large amount of commits that are not relevant for my platform. A changelog of a few commits on a patch release for the operators current lifecycle version is going to be a much safer ask IMO.

[yaelharel]

More places that our code path depends on the buildpack API:

Set default = true for web processes when platformAPI >= 0.6 and buildpackAPI < 0.6
https://github.com/buildpacks/lifecycle/blob/b8e2767db739fc96c3bf20b06f2a26adc76dc4cc/builder.go#L120-L124

Decode the LayerMetadataFile based on the buildpack API
I think that the interface in this code really helps but maybe there is a better way to do it:
https://github.com/buildpacks/lifecycle/blob/b8e2767db739fc96c3bf20b06f2a26adc76dc4cc/buildpack/files.go#L242-L246

All of the build.go file is full of if conditions based on the buildpack API:
https://github.com/buildpacks/lifecycle/blob/main/buildpack/build.go