chore: updated Makefile and mmv builder #12314

wyardley · 2024-11-13T06:28:57Z

Chain the two go run commands in the mmv1 make target with && (edit: did a narrower fix here than in the initial PR)
With this, the error in the linked ticket will now exit on provider build failure instead of continuing to the next step and exiting 0:

F1112 22:07:54.227629   53015 examples.go:300] template: artifact_registry_repository.go.tmpl:12: function "project" not defined
2024/11/12 22:07:54 products/dialogflowcx not specified, skipping generation
2024/11/12 22:07:54 products/iambeta not specified, skipping generation
2024/11/12 22:07:54 products/securitycenter not specified, skipping generation
2024/11/12 22:07:54 products/privateca not specified, skipping generation
exit status 1
make[1]: *** [mmv1] Error 1
make: *** [provider] Error 2
% echo $?
2

Added the product being built in the case where forceProvider is set to an empty string, and changed prefix to "Building" vs "Using"

This avoids the extra space and missing provider type that show up when building the default (Terraform) provider:

2024/11/12 20:14:47 Using ga version
2024/11/12 20:14:47 Using  provider

in favor of:

2024/11/12 22:23:30 Building ga version
2024/11/12 22:23:30 Building default (terraform) provider

Also fixed missing space here:

2024/11/12 22:40:26 Unexpected output folder (--version) detectedwhen deciding to add HashiCorp copyright headers.

Fixes hashicorp/terraform-provider-google#20317

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

github-actions · 2024-11-13T06:29:44Z

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@slevenick, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

slevenick · 2024-11-13T16:44:00Z

Requesting review from @c2thorn who has more knowledge of this file than I do

wyardley · 2024-11-13T16:59:22Z

Requesting review from @c2thorn who has more knowledge of this file than I do

Thanks. Yeah, there may be some quirks, so for sure good to confirm that this will make things better and not worse. I'm happy to try and implement in a narrower way if there are concerns.

I uncovered some other weirdness with some of the targets (like clean-provider and some other targets not having a guard for OUTPUT_PATH being set, clean-provider being confused by vim swapfiles, etc.)

c2thorn · 2024-11-14T18:19:23Z

GNUmakefile

@@ -1,3 +1,5 @@
+SHELL=/usr/bin/env bash -eo pipefail


To be honest I'm not sure how I would verify that using SHELL here break some part of CI or break in somebody's developer environment that it was previously working in. I'm also having trouble finding documentation/references for it, and I haven't used it myself so I don't have previous experience with it personally. The risk to benefit ratio isn't looking worthwhile for me

Is there another way to do this only in bash?

Only in bash, meaning fail on error only in the case where bash is available, or do you mean wrapping the build steps in the Make target with a bash script?

There could be some narrower fixes where we only adjust this specific target vs. setting SHELL for the makefile as a whole.

These two links have some information / possible ways to do it
https://stackoverflow.com/questions/23079651/equivalent-of-pipefail-in-gnu-make
https://stackoverflow.com/questions/33925523/how-to-set-pipefail-in-a-makefile

For that matter, both of the scripts in scripts/ are already bash specific (and in the case of scripts/doctor, hard-coded to /bin/bash vs using /usr/bin/env bash).

The risk to benefit ratio isn't looking worthwhile for me

I think the big risk here IMO is that someone doesn't notice a fatal build error and thinks it's succeeded. This could potentially cause evergreen builds in CI as well, if they're using the Makefile.

I would think that any modern system people are using for this should have at least some version of bash available somewhere, even if it's not in /bin. It is possible that some targets might need some adjustments later if a failing status somewhere causes issues, but I would guess most / all of these cases could be fixed.

I get what you're saying though -- happy to try for a narrower fix if you think that's better. Maybe it would work to do a && b instead of a ; b for the two go run commands in the mmv1 build target? In a quick test, that seems to work.

diff --git a/GNUmakefile b/GNUmakefile index 118b5aabe..9307c6f1a 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -61,8 +61,7 @@ terraform build provider: mmv1: cd mmv1;\ if [ "$(VERSION)" = "ga" ]; then \ - go run . --output $(OUTPUT_PATH) --version ga --no-docs $(mmv1_compile); \ - go run . --output $(OUTPUT_PATH) --version beta --no-code $(mmv1_compile); \ + go run . --output $(OUTPUT_PATH) --version ga --no-docs $(mmv1_compile) && go run . --output $(OUTPUT_PATH) --version beta --no-code $(mmv1_compile); \ else \ go run . --output $(OUTPUT_PATH) --version $(VERSION) $(mmv1_compile); \ fi

While it of course isn't possible to test everyone's individual build environment, I do think there are two kinds of tests that could be done:

Someone from Google / Hashicorp could run critical CI builds that don't run on PRs in a separate branch (by adjusting branch filters) to make sure they still function as expected

You may be aware of the common build tasks developers run locally other than basic building of the provider, so could probably run through some of those tasks and make sure that they all work.

@c2thorn see if you think the latest commit looks better / narrower. I also updated the PR description to match.

github-actions · 2024-11-19T09:42:11Z

@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

github-actions · 2024-11-21T09:42:25Z

@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

slevenick · 2024-11-22T22:54:06Z

Removed myself from review as I don't have much context on the new build files

github-actions · 2024-11-28T09:42:51Z

@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 2 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

wyardley · 2024-12-02T02:22:55Z

@c2thorn any thoughts about the updated version?

github-actions · 2024-12-05T09:42:35Z

@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 3 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

c2thorn

Apologies, I was OOO. @wyardley this seems like a viable alternative, thank you. I'll run the build.

c2thorn · 2024-12-09T17:31:00Z

/gcbrun

wyardley · 2024-12-09T17:40:47Z

Thanks!

I suggested a similar solution in #12341, which I think would have had a similar problem.

c2thorn · 2024-12-09T20:44:20Z

Build will not run on the branch since it affects the makefile. I will test this offline and probably merge tomorrow just so it won't collide with the release today. Setting a calendar reminder to do so

c2thorn

getting the following on my machine when testing

camthornton-mac:magic-modules camthornton$ make mmv1 VERSION=ga OUTPUT_PATH="$GOPATH/src/github.com/hashicorp/terraform-provider-google"
cd mmv1;\
		if [ "ga" = "ga" ]; then \
			# Chaining these with "&&" is critical here to prevent this
/bin/sh: -c: line 1: syntax error: unexpected end of file
make: *** [mmv1] Error 2

Removing the comments fixes this. Or we can move them above the cd mmv1;\ line, I think folks would be able to understand the lines it is referring to

wyardley · 2024-12-10T19:57:12Z

Weird, maybe I didn’t test again after adding the comments, or some slight difference in our setup. Yes, I can make that change and will test again locally as well.

Fixes hashicorp/terraform-provider-google#20317

wyardley · 2024-12-10T23:27:25Z

Thanks for testing again. Verified the issue, updated the comment, and tested build again locally.

github-actions · 2024-12-13T09:42:27Z

@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

github-actions · 2024-12-17T09:42:47Z

@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

wyardley · 2024-12-21T02:20:36Z

Hi @c2thorn just bugging you here since I think the automatic ones are still bugging Sam instead.

github-actions · 2024-12-24T09:41:27Z

@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 2 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

github-actions · 2024-12-31T09:41:12Z

@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 3 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

c2thorn

Got around to testing this, and it seems to be working now. Thanks @wyardley

github-actions bot requested a review from slevenick November 13, 2024 06:29

slevenick requested a review from c2thorn November 13, 2024 16:43

c2thorn reviewed Nov 14, 2024

View reviewed changes

wyardley force-pushed the wyardley/issues_20317 branch from e2c156c to 7dbc99d Compare November 14, 2024 18:37

github-actions bot requested a review from c2thorn November 14, 2024 18:37

wyardley mentioned this pull request Nov 15, 2024

Add tgc_v6 provider #12341

Merged

slevenick removed their request for review November 22, 2024 22:53

c2thorn reviewed Dec 9, 2024

View reviewed changes

github-actions bot requested a review from c2thorn December 9, 2024 17:41

c2thorn requested changes Dec 10, 2024

View reviewed changes

github-actions bot requested a review from c2thorn December 10, 2024 19:57

wyardley added 6 commits December 10, 2024 15:24

chore: update Makefile and mmv builder

2fca320

Fixes hashicorp/terraform-provider-google#20317

add missing space

4c0bca9

Switch to /usr/bin/env for shebang

7efb480

Narrower fix for not catching mmv1 build errors

65bcee3

break up lines and add comment

a3492d4

Fix comment and adjust wording

df7244b

wyardley force-pushed the wyardley/issues_20317 branch from 4bf3e01 to df7244b Compare December 10, 2024 23:27

c2thorn approved these changes Jan 6, 2025

View reviewed changes

c2thorn merged commit 4bee7ca into GoogleCloudPlatform:main Jan 6, 2025
5 of 7 checks passed

This was referenced Jan 6, 2025

chore: updated Makefile and mmv builder hashicorp/terraform-provider-google#20827

Merged

chore: updated Makefile and mmv builder hashicorp/terraform-provider-google-beta#9002

Merged

dkarwowski pushed a commit to dkarwowski/magic-modules that referenced this pull request Jan 7, 2025

chore: updated Makefile and mmv builder (GoogleCloudPlatform#12314)

d424071

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: updated Makefile and mmv builder #12314

chore: updated Makefile and mmv builder #12314

wyardley commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024

slevenick commented Nov 13, 2024

wyardley commented Nov 13, 2024

c2thorn Nov 14, 2024

wyardley Nov 14, 2024

wyardley Nov 14, 2024

github-actions bot commented Nov 19, 2024

github-actions bot commented Nov 21, 2024

slevenick commented Nov 22, 2024

github-actions bot commented Nov 28, 2024

wyardley commented Dec 2, 2024

github-actions bot commented Dec 5, 2024

c2thorn left a comment

c2thorn commented Dec 9, 2024

wyardley commented Dec 9, 2024

c2thorn commented Dec 9, 2024

c2thorn left a comment

wyardley commented Dec 10, 2024

wyardley commented Dec 10, 2024

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 17, 2024

wyardley commented Dec 21, 2024

github-actions bot commented Dec 24, 2024

github-actions bot commented Dec 31, 2024

c2thorn left a comment

chore: updated Makefile and mmv builder #12314

chore: updated Makefile and mmv builder #12314

Conversation

wyardley commented Nov 13, 2024 • edited Loading

github-actions bot commented Nov 13, 2024

slevenick commented Nov 13, 2024

wyardley commented Nov 13, 2024

c2thorn Nov 14, 2024

Choose a reason for hiding this comment

wyardley Nov 14, 2024

Choose a reason for hiding this comment

wyardley Nov 14, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024

github-actions bot commented Nov 21, 2024

slevenick commented Nov 22, 2024

github-actions bot commented Nov 28, 2024

wyardley commented Dec 2, 2024

github-actions bot commented Dec 5, 2024

c2thorn left a comment

Choose a reason for hiding this comment

c2thorn commented Dec 9, 2024

wyardley commented Dec 9, 2024

c2thorn commented Dec 9, 2024

c2thorn left a comment

Choose a reason for hiding this comment

wyardley commented Dec 10, 2024

wyardley commented Dec 10, 2024

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 17, 2024

wyardley commented Dec 21, 2024

github-actions bot commented Dec 24, 2024

github-actions bot commented Dec 31, 2024

c2thorn left a comment

Choose a reason for hiding this comment

wyardley commented Nov 13, 2024 •

edited

Loading