Suggestions to avoid repetition of common bash functions?

[jcbhmr]

This is an extension of https://github.com/devcontainers-contrib/features/discussions/83 that I thought I would pose to you, the official @devcontainers org, to see how you have thought of doing it.

You have the same problem as us over in @devcontainers-contrib, though on a smaller scale (you have 28 packages, we have 73). Too many install scripts declare a check_packages function. If/when that function needs to be changed, it's hard to do it to every single feature. They're all in different spots, some have indentation different, etc.

Have you thought about centralizing that logic into a single check_packages.lib.sh or similar? I notice that you have 26x hits when searching for "check_packages" on GitHub 😆, so you obviously have a similar problem as us.

Right now my (@jcbhmr's) idea is to use source ./vendor/*.sh and copy a single common vendor/ folder into each src/*/ folder to make src/*/vendor/*.sh files exist in each and every single feature image just prior to building/publishing. https://github.com/devcontainers-contrib/features/discussions/83#discussioncomment-4279546 https://github.com/jcbhmr/devcontainers-contrib-features-83-demo

@DaneWeber pointed out that this is stepping dangerously close to dependency management territory 😬.

My alarm bells are now ringing, while previously I was just noticing some warning signs. This shared-logic opens up the whole world of dependency management.

These features are already slippery because we're shelling out to scripts served up from unversioned URLs. Some of @jcbhmr's proposals above are definitely convenient for DRY code and ease of making an update, albeit with the difficulty of maintaining weakly-tested code that always runs against the latest shared version. Something like check_packages() seems pretty stable today, but there's going to be some breaking change in the next few years to something in the shared library.

The first idea that comes to me as a relatively small modification to the proposal would be this:

• Include version numbers, but just one number incremented on breaking changes: source header_v1.sh today and eventually source header_v8372.sh in the far future.
• The rule is that you can add new functions to the header without incrementing, but any change to an existing function requires incrementing the version number.
• Unfortunately, the build process now needs to know which version to inject. I'm thinking we include a magic comment in the install.sh file that specifies it and can be easily extracted in the build process: # BUILD DIRECTIVE: require header_v8.sh
• Also unfortunately, we now need to hang on to old versions until they are no longer used by any Feature. The build process could collect a list of all required header versions and even auto-open a PR to delete old versions that are no longer used.

After typing out ☝️ I'm starting to regret this.

@danielbraun89 was previously (and still is) using Cookiecutter templates to generate each feature from a template install.sh file that includes a single def of check_packages. This is another possibility. https://github.com/devcontainers-contrib/features/tree/main/feature_specs https://github.com/devcontainers-contrib/features/tree/main/pkgs/feature-template

TL;DR: I want to involve some actual cool guys from the official @devcontainers org in our discussion. We are looking for other suggestions. I am NOT proposing that you, the @devcontainers/features repo, adopt this! I'm just asking for an outside professional perspective.

The goal is to avoid repetition of check_packages definitions in source code.

[jcbhmr]

After combing through some more issues, it appears that there is some discussion in devcontainers/spec#129.

[joshspicer]

Thanks for opening up this discussion, and I appreciate the idea you and others have been putting forward! This is something the team and I are currently investigating. I hope to share more details via a formal spec proposal soon (likely after the holidays, though).

We're currently looking at this alongside devcontainers/spec#109, as there is likely some overlap here.