Remove the need for custom Make targets

[t0yv0]

Currently pulumi-awsx and pulumi-aws avail themselves of the following extension point to customize the makefile:

# Permit providers to extend the Makefile with provider-specific Make includes.
include $(wildcard .mk/*.mk)

This PR builds up more "formal" extension points that could be used instead so we can remove this include. This will make making changes to ci-mgmt more predictable.

[t0yv0]

Found that export transitively affects all sub-targets, which is unfortunate and not needed. Passing it more precisely is more predictable.

[t0yv0]

Here is an AWSX PR that uses these changes to remove .mk/foo.mk extenders:

pulumi/pulumi-awsx#1485

[t0yv0]

Here's an AWS PR that moves minimal schema computation from a Make extender into a script:

pulumi/pulumi-aws#5110

[corymhall]

Would it be possible to have it run the user provided build_provider_cmd and then automatically call the build_provider_cmd_default? It looks like with this each provider has to manually include this in their override. If the default cmd changes then we have to manually update everywhere a user has provided an override.

[t0yv0]

I'm not sure I see a problem. Calling build_provider_cmd_default is not appropriate for providers like AWSX.

• Bridged providers do not have any overrides; they receive the build_provider_cmd_default behavior
• Providers like AWSX ignore build_provider_cmd_default and override build_provider_cmd with yarn recipe
• Providers like AWS override build_provider_cmd to still run build_provider_cmd_default but inject a "before" hook to compute the minimal schema; this is rather a special case

This means that build_provider_cmd_default is managed by ci-mgmt entirely, but it's available for overriding in with something that injects pre/post hooks or wraps it.

[t0yv0]

I could delete build_provider_cmd_default and just inline it into AWS.

[corymhall]

The current approach seems too brittle and I think could lead to breakages in the future. For example:

Today we have:

# In ci-mgmt
build_provider_cmd = $(call build_provider_cmd_default,$(1),$(2),$(3))

# in aws
buildProviderCmd: "(VERSION=${VERSION_GENERIC} ./scripts/minimal_schema.sh); $(call build_provider_cmd_default,$(1),$(2),$(3))"

Then what if in 6 months someone updates ci-mgmt to have:

build_provider_cmd = $(call build_provider_cmd_default,$(1),$(2),$(3),$(4))

That person would have to know that the aws provider is customizing that target and update buildProviderCmd. If we inline it into the aws provider then I think we are in a worse position because we would drift over time as we update the default cmd.

[corymhall]

What if we added another hook, something like preDefaultProviderCmd?

[t0yv0]

I'm amenable though since YAGNI - we don't have use cases for those yet, not really.

[t0yv0]

Just I don't see this as any less brittle, you won't be able to just do this $(call build_provider_cmd_default,$(1),$(2),$(3),$(4)) anyway as that's a breaking change.

[corymhall]

Just I don't see this as any less brittle, you won't be able to just do this $(call build_provider_cmd_default,$(1),$(2),$(3),$(4)) anyway as that's a breaking change.

Why would that be a breaking change, isn't that what this PR is doing? You used to do

$(call build_provider_cmd,$(WORKING_DIR)/bin/$(PROVIDER))

and now you do

$(call build_provider_cmd,$(shell go env GOOS),$(shell go env GOARCH),$(WORKING_DIR)/bin/$(PROVIDER))

Could we not change this again in the future?

[t0yv0]

Sure this PR is breaking this interface and I intend to fix it with a fast-follow. GOOS and GOARCH are now passed explicitly rather than through env vars. I think that's better. So long as we are not doing too many breaking changes I think these are tolerable, this just creates work in the downstream repos.

[danielrbradley]

I can see the benefit in adding the pre... hook as it wouldn't require the positional arguments, which makes it simpler for providers like aws which are doing platform-independant actions.

While I do like the explicitness of the arguments for avoiding polluting the ENV, I agree it makes the interface more fragile as it doesn't allow for new arguments without a breaking change.

An alternative in-between option is to set non-tool-specific env variables: PROVIDER_BUILD_OUT, PROVIDER_BUILD_OS, PROVIDER_BUILD_ARCH. These are specific enough that they wouldn't affect the default behaviour of downstream tools, but are more amenable to changes in the future. The downside of this though is that the ENV vars are still completely invisible to the logs, making it harder to diagnose failures by copying and re-runing specific statements from the makefile output.

Overall, my vote would be to stick with the positional arguments for the purpose of completely overriding the build, but add the pre- hook for AWS's use case.

[t0yv0]

The context to this change is a convo we had with Daniel this morning.

Customizing Make per-provider is wanted for reasons in #1131 however the exact protocol suggested in 1311 for customizing Make is a little suspect. We seem to be using make --touch across partial graphs across CI jobs for example, which plays really unpredictably with custom new targets. It appears as of this morning we'd like to keep that and not back this out; in which case we'd really be better off without custom targets.

Without custom targets the Make extension idea boils down to some documented interfaces where a provider can plug in custom code to execute. One could be have Make auto-detect a ./script/foobar.sh and executing it, another way presented in this PR is a provider reconfigures part of the Makefile recipe in ci-mgmt.yaml to redefine key scripts with documented interfaces that are part of the build process. In a way this is yet more explicit than discovering ./script/foobar.sh.

I'd like us to consider going this way for now. Whatever interface there is going to be for extending the build process, it will be subject to breaking vs non-breaking changes. It was argued we should just remove customization entirely but that's at odds with on-boarding awsx, eks, and api-gateway quickly so I suggest we go with this extension interface, onboard the providers and keep simplifying afterward until perhaps one day no build customizers are needed.

[danielrbradley]

Overall happy with the approaches here.

Suggested tweaks from inline comments:

1. Add the preBuild hook for AWS so they don't have to interact with the BuildProviderCmd multi-platform complexities.
2. Change RenovateCmd into RenovateTarget.

[danielrbradley]

I can see the benefit in adding the pre... hook as it wouldn't require the positional arguments, which makes it simpler for providers like aws which are doing platform-independant actions.

While I do like the explicitness of the arguments for avoiding polluting the ENV, I agree it makes the interface more fragile as it doesn't allow for new arguments without a breaking change.

An alternative in-between option is to set non-tool-specific env variables: PROVIDER_BUILD_OUT, PROVIDER_BUILD_OS, PROVIDER_BUILD_ARCH. These are specific enough that they wouldn't affect the default behaviour of downstream tools, but are more amenable to changes in the future. The downside of this though is that the ENV vars are still completely invisible to the logs, making it harder to diagnose failures by copying and re-runing specific statements from the makefile output.

Overall, my vote would be to stick with the positional arguments for the purpose of completely overriding the build, but add the pre- hook for AWS's use case.

[danielrbradley]

As discussed, I think this is a backwards way of configuring renovate - we should just be building an existing makefile target via the per-repo renovate config instead.

The renovate target is fine as a temporary workaround until we figure out why renovate won't let us call make generate.

One suggestion here for simplifying would be to not make it an arbirary command but make it .Config.RenovateTarget instead so it's not arbitary code, but just an alias to an existing target.

This would also simplify the readability of the code as it's just creating an alias to an existing target:

Suggested change

	#{{- if .Config.RenovateCmd }}#
	renovate_cmd = #{{ .Config.RenovateCmd }}#
	renovate:
	$(call renovate_cmd)
	.PHONY: renovate
	#{{- end }}#
	#{{- if .Config.RenovateTarget }}#
	renovate: #{{ .Config.RenovateTarget }}#
	#{{- end }}#

Having the configuration as an existing target will also make it trivial in the future to remove this target and instead generate the renovate config with the call to make #{{ .Config.RenovateTarget }}# directly.

[t0yv0]

I like make renovate target TBH. This makes the interface between sub-systems more about the intent and less about the implementation which feels good.

[t0yv0]

The suggested change does make https://github.com/pulumi/pulumi-awsx/pull/1485/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R381 more difficult - we're not just generating SDKs, renovate might need to do something else specific to the repo. I think I will leave the Cmd hook. The Target hook seems to invite backdoors such as:

Config.RenovateTarget = "generate_sdks\n\techo STUFF\n\npost_renovate: renovate"