-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the need for custom Make targets #1292
Changes from 7 commits
0b5372d
6e05216
3f082b5
0bafe9b
b8e723b
a11bf10
8f11534
278df6f
3b4cfa3
07fac7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -255,32 +255,40 @@ lint_provider.fix: | |||||||||||||||||||
cd provider && golangci-lint run --path-prefix provider -c ../.golangci.yml --fix | ||||||||||||||||||||
.PHONY: lint_provider lint_provider.fix | ||||||||||||||||||||
|
||||||||||||||||||||
# `make provider_no_deps` builds the provider binary directly, without ensuring that | ||||||||||||||||||||
# `cmd/pulumi-resource-#{{ .Config.Provider }}#/schema.json` is valid and up to date. | ||||||||||||||||||||
# To create a release ready binary, you should use `make provider`. | ||||||||||||||||||||
build_provider_cmd_default = cd provider && GOOS=$(1) GOARCH=$(2) CGO_ENABLED=0 go build $(PULUMI_PROVIDER_BUILD_PARALLELISM) -o "$(3)" -ldflags "$(LDFLAGS)" $(PROJECT)/$(PROVIDER_PATH)/cmd/$(PROVIDER) | ||||||||||||||||||||
#{{- if .Config.BuildProviderCmd }}# | ||||||||||||||||||||
build_provider_cmd = #{{ .Config.BuildProviderCmd }}# | ||||||||||||||||||||
#{{- else }}# | ||||||||||||||||||||
build_provider_cmd = cd provider && CGO_ENABLED=0 go build $(PULUMI_PROVIDER_BUILD_PARALLELISM) -o "$(1)" -ldflags "$(LDFLAGS)" $(PROJECT)/$(PROVIDER_PATH)/cmd/$(PROVIDER) | ||||||||||||||||||||
build_provider_cmd = $(call build_provider_cmd_default,$(1),$(2),$(3)) | ||||||||||||||||||||
#{{- end }}# | ||||||||||||||||||||
|
||||||||||||||||||||
provider: bin/$(PROVIDER) | ||||||||||||||||||||
|
||||||||||||||||||||
# `make provider_no_deps` builds the provider binary directly, without ensuring that | ||||||||||||||||||||
# `cmd/pulumi-resource-#{{ .Config.Provider }}#/schema.json` is valid and up to date. | ||||||||||||||||||||
# To create a release ready binary, you should use `make provider`. | ||||||||||||||||||||
provider_no_deps: | ||||||||||||||||||||
$(call build_provider_cmd,$(WORKING_DIR)/bin/$(PROVIDER)) | ||||||||||||||||||||
$(call build_provider_cmd,$(shell go env GOOS),$(shell go env GOARCH),$(WORKING_DIR)/bin/$(PROVIDER)) | ||||||||||||||||||||
bin/$(PROVIDER): .make/schema | ||||||||||||||||||||
$(call build_provider_cmd,$(WORKING_DIR)/bin/$(PROVIDER)) | ||||||||||||||||||||
$(call build_provider_cmd,$(shell go env GOOS),$(shell go env GOARCH),$(WORKING_DIR)/bin/$(PROVIDER)) | ||||||||||||||||||||
.PHONY: provider provider_no_deps | ||||||||||||||||||||
|
||||||||||||||||||||
test: export PATH := $(WORKING_DIR)/bin:$(PATH) | ||||||||||||||||||||
test: | ||||||||||||||||||||
cd examples && go test -v -tags=all -parallel $(TESTPARALLELISM) -timeout 2h | ||||||||||||||||||||
.PHONY: test | ||||||||||||||||||||
|
||||||||||||||||||||
#{{- if .Config.TestProviderCmd }}# | ||||||||||||||||||||
test_provider_cmd = #{{ .Config.TestProviderCmd }}# | ||||||||||||||||||||
#{{- else }}# | ||||||||||||||||||||
test_provider_cmd = cd provider && go test -v -short \ | ||||||||||||||||||||
-coverprofile="coverage.txt" \ | ||||||||||||||||||||
-coverpkg="./...,github.com/hashicorp/terraform-provider-..." \ | ||||||||||||||||||||
-parallel $(TESTPARALLELISM) \ | ||||||||||||||||||||
./... | ||||||||||||||||||||
#{{- end }}# | ||||||||||||||||||||
test_provider: | ||||||||||||||||||||
cd provider && go test -v -short \ | ||||||||||||||||||||
-coverprofile="coverage.txt" \ | ||||||||||||||||||||
-coverpkg="./...,github.com/hashicorp/terraform-provider-..." \ | ||||||||||||||||||||
-parallel $(TESTPARALLELISM) \ | ||||||||||||||||||||
./... | ||||||||||||||||||||
$(call test_provider_cmd) | ||||||||||||||||||||
.PHONY: test_provider | ||||||||||||||||||||
|
||||||||||||||||||||
tfgen: schema | ||||||||||||||||||||
|
@@ -370,18 +378,18 @@ SKIP_SIGNING ?= | |||||||||||||||||||
|
||||||||||||||||||||
# These targets assume that the schema-embed.json exists - it's generated by tfgen. | ||||||||||||||||||||
# We disable CGO to ensure that the binary is statically linked. | ||||||||||||||||||||
bin/linux-amd64/$(PROVIDER): export GOOS := linux | ||||||||||||||||||||
bin/linux-amd64/$(PROVIDER): export GOARCH := amd64 | ||||||||||||||||||||
bin/linux-arm64/$(PROVIDER): export GOOS := linux | ||||||||||||||||||||
bin/linux-arm64/$(PROVIDER): export GOARCH := arm64 | ||||||||||||||||||||
bin/darwin-amd64/$(PROVIDER): export GOOS := darwin | ||||||||||||||||||||
bin/darwin-amd64/$(PROVIDER): export GOARCH := amd64 | ||||||||||||||||||||
bin/darwin-arm64/$(PROVIDER): export GOOS := darwin | ||||||||||||||||||||
bin/darwin-arm64/$(PROVIDER): export GOARCH := arm64 | ||||||||||||||||||||
bin/windows-amd64/$(PROVIDER).exe: export GOOS := windows | ||||||||||||||||||||
bin/windows-amd64/$(PROVIDER).exe: export GOARCH := amd64 | ||||||||||||||||||||
bin/linux-amd64/$(PROVIDER): GOOS := linux | ||||||||||||||||||||
bin/linux-amd64/$(PROVIDER): GOARCH := amd64 | ||||||||||||||||||||
bin/linux-arm64/$(PROVIDER): GOOS := linux | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found that |
||||||||||||||||||||
bin/linux-arm64/$(PROVIDER): GOARCH := arm64 | ||||||||||||||||||||
bin/darwin-amd64/$(PROVIDER): GOOS := darwin | ||||||||||||||||||||
bin/darwin-amd64/$(PROVIDER): GOARCH := amd64 | ||||||||||||||||||||
bin/darwin-arm64/$(PROVIDER): GOOS := darwin | ||||||||||||||||||||
bin/darwin-arm64/$(PROVIDER): GOARCH := arm64 | ||||||||||||||||||||
bin/windows-amd64/$(PROVIDER).exe: GOOS := windows | ||||||||||||||||||||
bin/windows-amd64/$(PROVIDER).exe: GOARCH := amd64 | ||||||||||||||||||||
bin/%/$(PROVIDER) bin/%/$(PROVIDER).exe: bin/jsign-6.0.jar | ||||||||||||||||||||
$(call build_provider_cmd,$(WORKING_DIR)/$@) | ||||||||||||||||||||
$(call build_provider_cmd,$(GOOS),$(GOARCH),$(WORKING_DIR)/$@) | ||||||||||||||||||||
|
||||||||||||||||||||
@# Only sign windows binary if fully configured. | ||||||||||||||||||||
@# Test variables set by joining with | between and looking for || showing at least one variable is empty. | ||||||||||||||||||||
|
@@ -440,5 +448,12 @@ provider_dist-windows-amd64: bin/$(PROVIDER)-v$(VERSION_GENERIC)-windows-amd64.t | |||||||||||||||||||
provider_dist: provider_dist-linux-amd64 provider_dist-linux-arm64 provider_dist-darwin-amd64 provider_dist-darwin-arm64 provider_dist-windows-amd64 | ||||||||||||||||||||
.PHONY: provider_dist-linux-amd64 provider_dist-linux-arm64 provider_dist-darwin-amd64 provider_dist-darwin-arm64 provider_dist-windows-amd64 provider_dist | ||||||||||||||||||||
|
||||||||||||||||||||
#{{- if .Config.RenovateCmd }}# | ||||||||||||||||||||
renovate_cmd = #{{ .Config.RenovateCmd }}# | ||||||||||||||||||||
renovate: | ||||||||||||||||||||
$(call renovate_cmd) | ||||||||||||||||||||
.PHONY: renovate | ||||||||||||||||||||
#{{- end }}# | ||||||||||||||||||||
Comment on lines
+456
to
+461
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 One suggestion here for simplifying would be to not make it an arbirary command but make it This would also simplify the readability of the code as it's just creating an alias to an existing target:
Suggested change
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||||||||||||||||||||
|
||||||||||||||||||||
# Permit providers to extend the Makefile with provider-specific Make includes. | ||||||||||||||||||||
include $(wildcard .mk/*.mk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to have it run the user provided
build_provider_cmd
and then automatically call thebuild_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I see a problem. Calling build_provider_cmd_default is not appropriate for providers like AWSX.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could delete build_provider_cmd_default and just inline it into AWS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach seems too brittle and I think could lead to breakages in the future. For example:
Today we have:
Then what if in 6 months someone updates ci-mgmt to have:
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we added another hook, something like
preDefaultProviderCmd
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm amenable though since YAGNI - we don't have use cases for those yet, not really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.