Skip to content

Commit

Permalink
Return more complex diff response to magician
Browse files Browse the repository at this point in the history
This is a prerequisite to hashicorp/terraform-provider-google#21287 because it will allow distinguishing between added, modified, and removed resources.

I also made the computation of the primary schema diff happen at build time, since it is a static value.
  • Loading branch information
melinath committed Feb 6, 2025
1 parent acd4916 commit d1a9c45
Show file tree
Hide file tree
Showing 13 changed files with 544 additions and 426 deletions.
22 changes: 15 additions & 7 deletions .ci/magician/cmd/generate_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ type diffCommentData struct {
Errors []Errors
}

type simpleSchemaDiff struct {
AddedResources, ModifiedResources, RemovedResources []string
}

const allowBreakingChangesLabel = "override-breaking-change"

var gcEnvironmentVariables = [...]string{
Expand Down Expand Up @@ -303,7 +307,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
data.MissingTests = missingTests
}

affectedResources, err := changedSchemaResources(diffProcessorPath, rnr)
affectedResources, err := computeAffectedResources(diffProcessorPath, rnr, repo)
if err != nil {
fmt.Println("computing changed resource schemas: ", err)
errors[repo.Title] = append(errors[repo.Title], "The diff processor crashed while computing changed resource schemas.")
Expand Down Expand Up @@ -460,27 +464,31 @@ func computeBreakingChanges(diffProcessorPath string, rnr ExecRunner) ([]Breakin
return changes, rnr.PopDir()
}

func changedSchemaResources(diffProcessorPath string, rnr ExecRunner) ([]string, error) {
func computeAffectedResources(diffProcessorPath string, rnr ExecRunner, repo source.Repo) ([]string, error) {
if err := rnr.PushDir(diffProcessorPath); err != nil {
return nil, err
}

output, err := rnr.Run("bin/diff-processor", []string{"changed-schema-resources"}, nil)
output, err := rnr.Run("bin/diff-processor", []string{"schema-diff"}, nil)
if err != nil {
return nil, err
}

fmt.Println("Resources with changed schemas: " + output)
fmt.Printf("Schema diff for %q: %s\n", repo.Name, output)

var labels []string
if err = json.Unmarshal([]byte(output), &labels); err != nil {
var simpleDiff simpleSchemaDiff
if err = json.Unmarshal([]byte(output), &simpleDiff); err != nil {
return nil, err
}

if err = rnr.PopDir(); err != nil {
return nil, err
}
return labels, nil
var resources []string
resources = append(resources, simpleDiff.AddedResources...)
resources = append(resources, simpleDiff.ModifiedResources...)
resources = append(resources, simpleDiff.RemovedResources...)
return resources, nil
}

// Run the missing test detector and return the results.
Expand Down
4 changes: 2 additions & 2 deletions .ci/magician/cmd/generate_comment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,11 @@ func TestExecGenerateComment(t *testing.T) {
{"/mock/dir/tfoics", "git", []string{"diff", "origin/auto-pr-123456-old", "origin/auto-pr-123456", "--shortstat"}, map[string]string(nil)},
{"/mock/dir/magic-modules/tools/diff-processor", "make", []string{"build"}, diffProcessorEnv},
{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"breaking-changes"}, map[string]string(nil)},
{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"changed-schema-resources"}, map[string]string(nil)},
{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"schema-diff"}, map[string]string(nil)},
{"/mock/dir/magic-modules/tools/diff-processor", "make", []string{"build"}, diffProcessorEnv},
{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"breaking-changes"}, map[string]string(nil)},
{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"detect-missing-tests", "/mock/dir/tpgb/google-beta/services"}, map[string]string(nil)},
{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"changed-schema-resources"}, map[string]string(nil)},
{"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"schema-diff"}, map[string]string(nil)},
},
} {
if actualCalls, ok := mr.Calls(method); !ok {
Expand Down
2 changes: 1 addition & 1 deletion .ci/magician/cmd/mock_runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func NewMockRunner() MockRunner {
"/mock/dir/magic-modules/.ci/magician git [clone -b auto-pr-123456 https://modular-magician:*******@github.com/modular-magician/terraform-provider-google-beta /mock/dir/tpgb] map[]": "",
"/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [breaking-changes] map[]": "",
"/mock/dir/magic-modules/tools/diff-processor make [build] " + sortedEnvString(diffProcessorEnv): "",
"/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [changed-schema-resources] map[]": "[\"google_alloydb_instance\"]",
"/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [schema-diff] map[]": "{\"AddedResources\": [\"google_alloydb_instance\"]}",
"/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [detect-missing-tests /mock/dir/tpgb/google-beta/services] map[]": `{"google_folder_access_approval_settings":{"SuggestedTest":"resource \"google_folder_access_approval_settings\" \"primary\" {\n uncovered_field = # value needed\n}","Tests":["a","b","c"]}}`,
"/mock/dir/tgc git [diff origin/auto-pr-123456-old origin/auto-pr-123456 --shortstat] map[]": " 1 file changed, 10 insertions(+)\n",
"/mock/dir/tgc git [fetch origin auto-pr-123456-old] map[]": "",
Expand Down
2 changes: 1 addition & 1 deletion tools/diff-processor/cmd/breaking_changes.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func newBreakingChangesCmd(rootOptions *rootOptions) *cobra.Command {
o := &breakingChangesOptions{
rootOptions: rootOptions,
computeSchemaDiff: func() diff.SchemaDiff {
return diff.ComputeSchemaDiff(oldProvider.ResourceMap(), newProvider.ResourceMap())
return schemaDiff
},
stdout: os.Stdout,
}
Expand Down
53 changes: 0 additions & 53 deletions tools/diff-processor/cmd/changed_schema_resources.go

This file was deleted.

165 changes: 0 additions & 165 deletions tools/diff-processor/cmd/changed_schema_resources_test.go

This file was deleted.

2 changes: 1 addition & 1 deletion tools/diff-processor/cmd/detect_missing_docs.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func newDetectMissingDocsCmd(rootOptions *rootOptions) *cobra.Command {
o := &detectMissingDocsOptions{
rootOptions: rootOptions,
computeSchemaDiff: func() diff.SchemaDiff {
return diff.ComputeSchemaDiff(oldProvider.ResourceMap(), newProvider.ResourceMap())
return schemaDiff
},
stdout: os.Stdout,
}
Expand Down
2 changes: 0 additions & 2 deletions tools/diff-processor/cmd/detect_missing_tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ func (o *detectMissingTestsOptions) run(args []string) error {
glog.Infof("error reading path: %s, err: %v", path, err)
}

schemaDiff := diff.ComputeSchemaDiff(oldProvider.ResourceMap(), newProvider.ResourceMap())

missingTests, err := detector.DetectMissingTests(schemaDiff, allTests)
if err != nil {
return fmt.Errorf("error detecting missing tests: %v", err)
Expand Down
1 change: 1 addition & 0 deletions tools/diff-processor/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ func newRootCmd() (*cobra.Command, *rootOptions, error) {
cmd.AddCommand(newBreakingChangesCmd(o))
cmd.AddCommand(newChangedSchemaResourcesCmd(o))
cmd.AddCommand(newDetectMissingTestsCmd(o))
cmd.AddCommand(newSchemaDiffCmd(o))
return cmd, o, nil
}

Expand Down
Loading

0 comments on commit d1a9c45

Please sign in to comment.