Further refactoring of magician VCR #11647

trodge · 2024-09-05T20:19:32Z

SA_KEY environment variable now optional, will add support for other credentials in follow-up PR.

Options struct for UploadLogs

logBucket and cassetteBucket moved into Tester object

safeToLog map added, environment variables must be marked as safe to be logged

prNumber parameter for UploadLogs and FetchCassettes is now replaced with opt.Head and head (head includes auto-pr- or auto-cl- prefix)

Release Note Template for Downstream PRs (will be copied)

modular-magician · 2024-09-05T20:28:48Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2024-09-06T00:35:23Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

melinath

a few change requests below. This is a lot of unrelated changes at once so I'm a little concerned I may have missed issues. Please be sure to exercise the VCR test logic (including record after replay) to make sure it's all working as expected.

.ci/magician/cmd/check_cassettes.go

.ci/magician/cmd/test_terraform_vcr.go

melinath · 2024-09-06T22:22:25Z

.ci/magician/cmd/vcr_cassette_update.go

@@ -90,7 +90,7 @@ var vcrCassetteUpdateCmd = &cobra.Command{
 		}
 		ctlr := source.NewController(env["GOPATH"], "hashicorp", env["GITHUB_TOKEN_CLASSIC"], rnr)

-		vt, err := vcr.NewTester(env, rnr)
+		vt, err := vcr.NewTester(env, "ci-vcr-logs", "ci-vcr-cassettes", rnr)


Based on the code, I'm not sure these are correct... but I'm also not sure they're being used here? We're not currently using this go file & there could be related bugs there already? attn @iyabchen @shuyama1

"ci-vcr-cassettes" was previously hard-coded into the FetchCassettes function. "ci-vcr-logs" would only be used if vt.UploadLogs were called.

I'm making logBucket an empty string for now since this command uses its own function to upload logs.

melinath · 2024-09-06T22:25:39Z

.ci/magician/exec/runner.go

@@ -100,6 +100,14 @@ func (ar *Runner) ReadFile(name string) (string, error) {
 	return string(data), nil
 }

+func (ar *Runner) AppendFile(name, data string) error {


Rather than reading & then writing the file, we may want to use append modes - more efficient: https://pkg.go.dev/os#example-OpenFile-Append

Does this look correct to you?

This function doesn't get called yet, so I'll save testing it for the next PR.

That looks about right; I'm not super familiar with doing this though.

.ci/magician/vcr/tester.go

melinath · 2024-09-06T22:34:36Z

.ci/magician/vcr/tester.go

 		}
 	}
 	vt.cassettePaths[version] = cassettePath
+	if len(fetchErrors) > 0 {
+		return fmt.Errorf("errors fetching cassettes: %v", fetchErrors)


I think the changes to the error handling here are incorrect as written. I'm fairly certain the fetches can fail in valid circumstances - for example, if a PR is opened there won't be any cassettes, so the cassettes for that PR will "fail" to be fetched. See https://pantheon.corp.google.com/cloud-build/builds;region=global/fadf17a3-a053-400a-871a-95e1d1b6251f;step=23?inv=1&invt=AbbbyA&project=graphite-docker-images line 12-16 for example.

Why do we need these changes? What would you think about holding them for a later PR?

We don't need to change the error handling now. I'll hold off for a later PR.

melinath · 2024-09-06T22:40:58Z

It could be worth splitting this into multiple smaller PRs.

modular-magician · 2024-09-06T23:02:41Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2024-09-06T23:27:35Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2024-09-11T18:34:50Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2024-09-12T23:53:47Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

roaks3

Generally speaking I don't see anything wrong here, but I would second what Stephen suggested about splitting this up into a few smaller PRs. The difficulty in reading changes like this all together is the context switching that I need to do as I'm reading line-by-line, paired with a things like signature changes, so it is hard for me to feel confident that I haven't missed something. FWIW I think it would save you effort by having each individual review move faster.

That said, pending enough testing that this feels safe, 👍 from me.

melinath

Could you split this into multiple PRs?

one for all the trivial changes like moving the template files
one per major set of changes (for example, one for changes to make uploadlogs take a struct, one for moving logBucket and cassetteBucket into the tester, one for replacing prnumber with head, etc.)

.ci/magician/cmd/test_terraform_vcr.go

modular-magician · 2024-09-13T21:04:34Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

* Reorganize template locations * Add AppendFile method to ExecRunner * Add Head, Version, and LogBucket to template data for templates that use PRNumber

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

modular-magician · 2024-09-20T16:07:25Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2024-09-20T16:19:58Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

melinath · 2024-09-24T17:50:31Z

.ci/magician/vcr/tester.go

@@ -156,34 +194,34 @@ type RunOptions struct {

 // Run the vcr tests in the given mode and provider version and return the result.
 // This will overwrite any existing logs for the given mode and version.
-func (vt *Tester) Run(opt RunOptions) (Result, error) {
-	logPath, err := vt.getLogPath(opt.Mode, opt.Version)
+func (vt *Tester) Run(opts RunOptions) (Result, error) {


Suggested change

func (vt *Tester) Run(opts RunOptions) (Result, error) {

func (vt *Tester) Run(opt RunOptions) (Result, error) {

Could this rename be excluded for clarity?

.ci/magician/vcr/tester.go

.ci/magician/cmd/vcr_cassette_update_test.go

.ci/magician/cmd/test_terraform_vcr.go

modular-magician · 2024-09-24T21:07:29Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2024-09-24T21:09:35Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2024-09-25T17:37:17Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

melinath

LGTM apart from a couple things that seem incorrect / unnecessary.

melinath · 2024-09-25T17:35:44Z

.ci/magician/cmd/test_terraform_vcr.go

+			AfterRecording: true,
+			Parallel:       false,


Suggested change

AfterRecording: true,

Parallel: false,

Parallel: true,

melinath · 2024-09-25T17:36:10Z

.ci/magician/cmd/test_terraform_vcr.go

+		AfterRecording: false,
+		Parallel:       false,


Suggested change

AfterRecording: false,

Parallel: false,

modular-magician · 2024-09-26T16:43:17Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

.ci/magician/cmd/test_terraform_vcr.go

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

modular-magician · 2024-09-26T18:26:24Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

modular-magician · 2024-09-26T18:29:26Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

shuyama1 · 2024-09-26T22:07:16Z

magician-vcr-eap

Is this intentional? It might be causing some of the check failures in the main.

#11846 fixes this.

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

trodge requested a review from roaks3 September 5, 2024 20:24

trodge marked this pull request as ready for review September 5, 2024 21:58

trodge requested review from a team and removed request for roaks3 September 6, 2024 18:15

trodge marked this pull request as draft September 6, 2024 18:17

trodge marked this pull request as ready for review September 6, 2024 18:17

trodge requested review from melinath and removed request for a team September 6, 2024 21:44

melinath requested changes Sep 6, 2024

View reviewed changes

trodge force-pushed the magician-vcr-more branch from c41b4b4 to 277b088 Compare September 11, 2024 18:24

roaks3 self-requested a review September 13, 2024 18:55

roaks3 reviewed Sep 13, 2024

View reviewed changes

trodge requested a review from melinath September 13, 2024 19:04

melinath requested changes Sep 13, 2024

View reviewed changes

.ci/magician/cmd/test_terraform_vcr.go Outdated Show resolved Hide resolved

trodge and others added 8 commits September 20, 2024 15:06

Use an allow list for env logging

f2aa285

rename prNumber to head in FetchCassettes

1a82625

more refactoring

0d4476c

More refactoring

4ed25d4

* Reorganize template locations * Add AppendFile method to ExecRunner * Add Head, Version, and LogBucket to template data for templates that use PRNumber

Update .ci/magician/cmd/test_terraform_vcr.go

4f397ad

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

Fix issues found in review

81ddf33

Correct the build step

ab41648

Hide the SA_KEY

43a520c

Use cassette bucket variable

e56f0f1

trodge requested a review from melinath September 20, 2024 16:13

melinath requested changes Sep 24, 2024

View reviewed changes

trodge added 2 commits September 24, 2024 13:57

Address review comments

5969a39

Fix comments

77ed354

Merge branch 'main' into magician-vcr-more

c52f41f

trodge requested a review from melinath September 25, 2024 17:29

melinath approved these changes Sep 25, 2024

View reviewed changes

Remove redundant defaults

b0a3247

melinath approved these changes Sep 26, 2024

View reviewed changes

.ci/magician/cmd/test_terraform_vcr.go Outdated Show resolved Hide resolved

trodge and others added 2 commits September 26, 2024 18:18

Update .ci/magician/cmd/test_terraform_vcr.go

3f08790

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

gofmt

9d59ba7

trodge merged commit 376778b into GoogleCloudPlatform:main Sep 26, 2024
5 checks passed

shuyama1 reviewed Sep 26, 2024

View reviewed changes

sebkalis pushed a commit to sebkalis/magic-modules that referenced this pull request Sep 27, 2024

Further refactoring of magician VCR (GoogleCloudPlatform#11647)

fa040fb

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

anavada pushed a commit to anavada/magic-modules that referenced this pull request Sep 30, 2024

Further refactoring of magician VCR (GoogleCloudPlatform#11647)

227a05e

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

niharika-98 pushed a commit to niharika-98/magic-modules that referenced this pull request Oct 7, 2024

Further refactoring of magician VCR (GoogleCloudPlatform#11647)

4e90e8d

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

trodge added a commit to trodge/magic-modules that referenced this pull request Oct 10, 2024

Further refactoring of magician VCR (GoogleCloudPlatform#11647)

b6b081c

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

Philip-Jonany pushed a commit to Philip-Jonany/magic-modules that referenced this pull request Nov 4, 2024

Further refactoring of magician VCR (GoogleCloudPlatform#11647)

d61f840

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further refactoring of magician VCR #11647

Further refactoring of magician VCR #11647

trodge commented Sep 5, 2024 •

edited

Loading

modular-magician commented Sep 5, 2024

modular-magician commented Sep 6, 2024

melinath left a comment

melinath Sep 6, 2024

trodge Sep 6, 2024 •

edited

Loading

melinath Sep 6, 2024

trodge Sep 9, 2024

melinath Sep 9, 2024

melinath Sep 6, 2024

trodge Sep 6, 2024

melinath commented Sep 6, 2024

modular-magician commented Sep 6, 2024

modular-magician commented Sep 6, 2024

modular-magician commented Sep 11, 2024

modular-magician commented Sep 12, 2024

roaks3 left a comment

melinath left a comment

modular-magician commented Sep 13, 2024

modular-magician commented Sep 20, 2024

modular-magician commented Sep 20, 2024

melinath Sep 24, 2024

trodge Sep 24, 2024

modular-magician commented Sep 24, 2024

modular-magician commented Sep 24, 2024

modular-magician commented Sep 25, 2024

melinath left a comment

melinath Sep 25, 2024

melinath Sep 25, 2024

modular-magician commented Sep 26, 2024

modular-magician commented Sep 26, 2024

modular-magician commented Sep 26, 2024

shuyama1 Sep 26, 2024 •

edited

Loading

melinath Sep 26, 2024

	func (vt *Tester) Run(opts RunOptions) (Result, error) {
	func (vt *Tester) Run(opt RunOptions) (Result, error) {

Further refactoring of magician VCR #11647

Further refactoring of magician VCR #11647

Conversation

trodge commented Sep 5, 2024 • edited Loading

modular-magician commented Sep 5, 2024

Diff report

modular-magician commented Sep 6, 2024

Diff report

melinath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trodge Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melinath commented Sep 6, 2024

modular-magician commented Sep 6, 2024

Diff report

modular-magician commented Sep 6, 2024

Diff report

modular-magician commented Sep 11, 2024

Diff report

modular-magician commented Sep 12, 2024

Diff report

roaks3 left a comment

Choose a reason for hiding this comment

melinath left a comment

Choose a reason for hiding this comment

modular-magician commented Sep 13, 2024

Diff report

modular-magician commented Sep 20, 2024

Diff report

modular-magician commented Sep 20, 2024

Diff report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Sep 24, 2024

Diff report

modular-magician commented Sep 24, 2024

Diff report

modular-magician commented Sep 25, 2024

Diff report

melinath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Sep 26, 2024

Diff report

modular-magician commented Sep 26, 2024

Diff report

modular-magician commented Sep 26, 2024

Diff report

shuyama1 Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trodge commented Sep 5, 2024 •

edited

Loading

trodge Sep 6, 2024 •

edited

Loading

shuyama1 Sep 26, 2024 •

edited

Loading