Adding extension support

[pacostas]

Summary

This PR adds the --extension flag option for packaging an extension.

Use Cases

With this PR is possible to package an extension by using below command.

jam pack --extension  ../path/to/extension.toml  --output ../path/to/build/extension.tgz --version "1.2.3"

Checklist

• I have viewed, signed, and submitted the Contributor License Agreement.
• I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
• I have added an integration test, if necessary.
• I have reviewed the styleguide for guidance on my code quality.
• I'm happy with the commit history on this PR (I have rebased/squashed as needed).

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: pacostas / name: Costas Papastathis (c310243, e151c0b, 56915c2, 3278986, 5970fe4, ee19f40, 08cea19, 0a09765, 3871592, cb18560, 132805d, d121d92, dc7d468, c427d5a)
• ✅ login: robdimsdale / name: Rob Dimsdale-Zucker (63d8d64)

[robdimsdale]

Thanks for submitting this @pacostas

We'll take a look at trying out this functionality and give you feedback on it.

In the meantime, could you sign the CLA (see above)?

[pacostas]

@robdimsdale I've added it as a draft, as the tests are not implemented yet and also I'm not sure if this heads to the right direction.
Probably next week I'll be able to sign the CLA.
Thank you :)

[sophiewigmore]

@pacostas to me, this PR seems like its definitely heading in the right direction!
I cloned https://github.com/nodeshift/ubi-nodejs-extension/, added pre-package = "./scripts/build.sh" to the extension.toml, and was able to build the extension. I haven't been able to actually use the extension yet but the packaging seems reasonable so far

[mhdawson]

Successfully used this in an integration test in https://github.com/nodeshift/ubi-nodejs-extension/pull/35

[sophiewigmore]

@pacostas let us know when this PR is officially ready for review or if you need feedback on anything particular

[pacostas]

@sophiewigmore I dont have anything in particular to ask, so I think is ready for review :)

[sophiewigmore]

Even though the cases are very similar, I'd love to see an extra test case added for the extension.toml case

[pacostas]

The final output, looks like it will end up with a lot of if statements handling the scenario "if is buildpack or extension" each time the buildpack and extension differ, something like:

if isBuildpack{
	config.Buildpack.Version = flags.version
} else {
config.Extension.Versions = flags.version
}

I think it would be better to create a separate method that will handle only the extensions. The only drawback with that is that it ends up with a few lines of duplicate code which compared to the multiple "if statements" solution seems more manageable.
I'm sharing in case you have any other suggestions you would like to see, in terms of a more maintainable solution.

[robdimsdale]

@pacostas

I think it would be better to create a separate method that will handle only the extensions. The only drawback with that is that it ends up with a few lines of duplicate code which compared to the multiple "if statements" solution seems more manageable.

I think this is fine. A few lines of duplicated code sounds potentially better than lots of if statements. We will be able to evaluate for sure once we see it, so I would suggest trying that option out and pushing it to this branch.

[pacostas]

@pacostas

I think it would be better to create a separate method that will handle only the extensions. The only drawback with that is that it ends up with a few lines of duplicate code which compared to the multiple "if statements" solution seems more manageable.

I think this is fine. A few lines of duplicated code sounds potentially better than lots of if statements. We will be able to evaluate for sure once we see it, so I would suggest trying that option out and pushing it to this branch.

Below commits is the implementation using a separate function.
f808f6f
6abd3ed
563b762

What is missing is that I havent added tests yet for packing the extension.
Also the HasStack method is missing, I will open soon another PR on packit to add the HasStack method, the tests will fail till then.

[pacostas]

This PR has to be merged after paketo-buildpacks/packit#490

[mhdawson]

For anybody looking paketo-buildpacks/packit#490 has already landed :)

[sophiewigmore]

@pacostas @mhdawson we will take another look this week! thanks for the updates

[pacostas]

@pacostas @mhdawson we will take another look this week! thanks for the updates
@sophiewigmore

I havent finish it yet, as I havent finished with unit/integration tests. I'll submit them soon almost 90% done

[pacostas]

Even though the cases are very similar, I'd love to see an extra test case added for the extension.toml case

@sophiewigmore I've added the test cases, similar to those that exist for buildpack.toml

[pacostas]

@sophiewigmore I've finished with this PR Implementation, is ready for review. Although this PR wont pass the tests till this PR paketo-buildpacks/packit#490 has being included on the latest release version of https://github.com/paketo-buildpacks/packit due to a recent change that I had to add.

[robdimsdale]

I released packit v2.10.1 which includes the HasStack method. You should be good to update to that and fix the tests.

Also, I think you can refactor the dependency_cacher.go file using an unexported common method and an unexported interface. See this commit for inspiration. I think this will make the code much easier to read, as there is a fair amount of duplication across the Cache/CacheExtension methods. Because this would be internal/unexported interfaces and functions, it doesn't change the exported interface and so nothing would change about the tests or the consumption of the library.

[pacostas]

@robdimsdale I've finished with the implementation of cache and cacheExtension in similar pattern as of the example you sent me on this commit . If the commit looks good, I can also proceed in refactoring fileBunlder in a similar way.

[robdimsdale]

I think this is almost there. There's a couple of types we can make unexported, which keeps them out of the public interface of the package, and there's another minor logging change I think will improve UX. All comments inline.

[pacostas]

@robdimsdale I've added your suggestions.

• 532c327
• 1e691b2

I've also refactored Bunlde and ExtensionBundle for avoiding duplicate code as @sophiewigmore has suggested

• 946a8d1

[sophiewigmore]

I'll take another look in the next few days, sorry about the delay

[sophiewigmore]

@pacostas these changes look great. Thanks for the hard work on this PR 🔥

[sophiewigmore]

I cannot merge it in due to conflicts, if you can resolve those

[pacostas]

@sophiewigmore There wasnt somewhere visible to me for resolving the conflicts, so I rebased it instead :)