Prepare to remove actions hooks

[danielrbradley]

This will allow us to remove all of our preTest hooks in provider ci-mgmt configs - which we'd like to get rid of to avoid arbitary code injection into workflows which make them fragile and hard to refactor. See also #1037

Related to #936

Add standalone option for running provider integration tests

Set integrationTestProvider: true to run go test [...] -tags=${{ matrix.language }} in the provider directory.

Example of existing preTest usage: https://github.com/pulumi/pulumi-gcp/blob/92b64b51bfb05fc0d2d7b9c1cbcafe7de8a6f7f3/.ci-mgmt.yaml#L43

Always prepare upstream before testing

This step is safe to run, as we already do before the Go build steps elsewhere. The upstream target is always there but might be a no-op.

Add SSH setup option

Set sshPrivateKey: ${{ secrets.PRIVATE_SSH_KEY_FOR_DIGITALOCEAN }} to set up SSH for testing.

Example of docker use of preTest: https://github.com/pulumi/pulumi-docker/blob/fc1d68f823c34cef72e34e060b093230e21fc636/.ci-mgmt.yaml#L35

Migration

1. Merge this PR with intent-focused config options
2. Open PRs to the ~28 providers to replace the preTest hook with alternative settings
   • Set integrationTestProvider: true on all providers which run integration tests via the provider module
   • Use setup-script for arbitrary bash commands before tests
   • Set sshPrivateKey: ${{ secrets.PRIVATE_SSH_KEY_FOR_DIGITALOCEAN }} for docker
3. Remove the actions config completely

Existing usage of actions hooks (there is no use of preBuild - only preTest): https://github.com/search?q=org%3Apulumi+path%3A.ci-mgmt.yaml+%22actions%3A%22&type=code

[blampe]

pulumi/pulumi-docker-build#82 is another example where we need some similar extensibility for a native provider. I didn't want to just shoehorn an if provider === "docker-build" ... exception -- I'd much rather have a real extension point and use that, so I'm happy you're thinking about this!

which we'd like to get rid of to avoid arbitary code injection into workflows which make them fragile and hard to refactor. See also #1037

I wonder if there's a better way to strike a balance between provider repos still having some control over their prerequisites while not being so tightly coupled to ci-mgmt. We can certainly add specific options like sshPrivateKey:, injectEnvVarsForDockerBuild:, etc., but in the limit we'll end up with a ton of one-off special cases. That adds up to a lot of accidental complexity -- ci-mgmt needs to know more about provider internals than it really should, and changing those internals becomes a coordination problem between the provider and ci-mgmt. Additionally, any time ci-mgmt needs a new option added it represents some friction that will nudge people towards hacky workarounds, inaction (as I've done with docker-build), or forks.

I think ideally provider authors should be able to "own" some of their CI setup without needing to know or care about how ci-mgmt orchestrates things. This is a big problem area, but one simple strategy we could consider is to treat make targets as more of a contract between the provider and ci-mgmt. Concretely:

• ci-mgmt ships a default Makefile to your repo, with all the build targets it expects to be defined, along with reasonable defaults.
• The Makefile can do something like -include Makefile.local to add local overrides from the repo, if they exist.
• The provider can customize a make ci-setup target in Makefile.local with their own logic if they need it.

You wouldn't be able to add arbitrary GH actions with that approach, but I agree that should probably be a non-goal anyway. Often times the GH actions we're talking about can be replicated with a few shell commands or a simple script. For example the docker-build provider has the same SSH key requirement as the docker provider, but that's addressable with a little bit of code that works equally well locally as in CI.

[blampe]

This is unit testing the provider, no?

It's natural for providers to have slightly different ways to test themselves (for example I think it's a mistake to assume the provider subdiretory). Simplifying this to something like make test_provider_ci (or whatever) could accomplish the same while still leaving the door open for some repo-specific behavior.

[danielrbradley]

This is running the language-specific tests within the provider. I think these are basically all tests which depend on either the SDKs or having access to deploy test infrastructure. These are specified by tags such as nodejs similar to the examples. These tests are excluded from running by default when running the normal provider unit tests.

[blampe]

Oh man I missed the language tag, that's weird! Is this maybe just an instance of someone dropping E2E tests under provider instead of examples?

[danielrbradley]

Correct - which is what we already have in a few providers. I think all the replay tests are in the provider folder, for example. This just formalises what's already being done into a simple option rather than leaning on providers to inject their own custom steps and matrix options.

[danielrbradley]

pulumi/pulumi-docker-build#82 is another example where we need some similar extensibility for a native provider. I didn't want to just shoehorn an if provider === "docker-build" ... exception -- I'd much rather have a real extension point and use that, so I'm happy you're thinking about this!

Indeed, provider-specific switches should be an absolute last resort! I think there's normally a way to generalise what we're trying to achive with a specific provider to describe the intent better. My leaning is that more smaller, more focused options are easier to reason against compared to the arbitrary hooks which then have to be carefully audited before each refactor.

I wonder if there's a better way to strike a balance between provider repos still having some control over their prerequisites while not being so tightly coupled to ci-mgmt. We can certainly add specific options like sshPrivateKey:, injectEnvVarsForDockerBuild:, etc., but in the limit we'll end up with a ton of one-off special cases. That adds up to a lot of accidental complexity -- ci-mgmt needs to know more about provider internals than it really should, and changing those internals becomes a coordination problem between the provider and ci-mgmt. Additionally, any time ci-mgmt needs a new option added it represents some friction that will nudge people towards hacky workarounds, inaction (as I've done with docker-build), or forks.

I agree we don't want too much sprawl, though right now I think we only need to introduce these 2 new options (integrationTestProvider and sshPrivateKey) in addition to the existing setup-script option.

I think ideally provider authors should be able to "own" some of their CI setup without needing to know or care about how ci-mgmt orchestrates things. This is a big problem area, but one simple strategy we could consider is to treat make targets as more of a contract between the provider and ci-mgmt. Concretely:

• ci-mgmt ships a default Makefile to your repo, with all the build targets it expects to be defined, along with reasonable defaults.
• The Makefile can do something like -include Makefile.local to add local overrides from the repo, if they exist.
• The provider can customize a make ci-setup target in Makefile.local with their own logic if they need it.

I like this idea - adding the more custom hooks via the makefile - it's somewhat easier to reason about the impact compared to changing CI workflows.

I discussed trying to formalise the interface between local builds and CI a while back in the provider build systems design.

Another simple way of implementing this could be via having the option to add scripts to call as part of different steps. E.g. scripts/provider_prebuild.sh or scripts/test_setup.sh. Ideally though I think we still want to lean towards implementing these cases for broader consumption. If a feature is needed in one provider, it's pretty likely that another will need something similar too at some point. Another option too is for some providers just to opt out of using the makefile generation and opt to manage that themselves, though this would be useful for bringing in new providers it would be quite a step backwards for bridged providers.

Leaning towards injecting commands rather than injecting actions is where I'm pushing towards for the near-future.

[thomas11]

I'm a bit concerned about this as well.

I wonder if there's a better way to strike a balance between provider repos still having some control over their prerequisites while not being so tightly coupled to ci-mgmt. We can certainly add specific options like sshPrivateKey:, injectEnvVarsForDockerBuild:, etc., but in the limit we'll end up with a ton of one-off special cases. That adds up to a lot of accidental complexity -- ci-mgmt needs to know more about provider internals than it really should, and changing those internals becomes a coordination problem between the provider and ci-mgmt. Additionally, any time ci-mgmt needs a new option added it represents some friction that will nudge people towards hacky workarounds, inaction (as I've done with docker-build), or forks.

Especially for SSH which is only used by one provider, correct?

I wouldn't block on this PR but maybe there are other options?

[danielrbradley]

Especially for SSH which is only used by one provider, correct?

I wouldn't block on this PR but maybe there are other options?

It is only currently used by one provider, though will probably need it for at least one more as we onboard the other providers.

There's already the .setup-scriptconfig option we can use here which allows a custom set of bash commands to be run before executing the tests in CI. I don't love the name, but this option for injecting arbitary command is easier to reason about than injecting GHA steps as it limits access to only the local file system and not for uploading/downloading assets or adding action dependencies etc.

SSH feels like a generic enough option to be generally useful.

[thomas11]

LGTM, discussion point about extensibility addressed in sync review