diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index 2ac0f7a0b8ca..fe1917b8f220 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -60,6 +60,17 @@ type MissingTestInfo struct { Tests []string } +type MissingDocInfo struct { + Name string + FilePath string + Fields []string +} + +type MissingDocsSummary struct { + Resource []MissingDocInfo + DataSource []MissingDocInfo +} + type Errors struct { Title string Errors []string @@ -71,6 +82,7 @@ type diffCommentData struct { BreakingChanges []BreakingChange MissingServiceLabels []string MissingTests map[string]*MissingTestInfo + MissingDocs *MissingDocsSummary Errors []Errors } @@ -310,6 +322,13 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep, errors[repo.Title] = append(errors[repo.Title], "The missing test detector failed to run.") } data.MissingTests = missingTests + + missingDocs, err := detectMissingDocs(diffProcessorPath, repo.Path, rnr) + if err != nil { + fmt.Println("Error running missing doc detector: ", err) + errors[repo.Title] = append(errors[repo.Title], "The missing doc detector failed to run.") + } + data.MissingDocs = missingDocs } simpleDiff, err := computeAffectedResources(diffProcessorPath, rnr, repo) @@ -549,6 +568,28 @@ func detectMissingTests(diffProcessorPath, tpgbLocalPath string, rnr ExecRunner) return missingTests, rnr.PopDir() } +// Run the missing doc detector and return the results. +func detectMissingDocs(diffProcessorPath, tpgbLocalPath string, rnr ExecRunner) (*MissingDocsSummary, error) { + if err := rnr.PushDir(diffProcessorPath); err != nil { + return nil, err + } + + output, err := rnr.Run("bin/diff-processor", []string{"detect-missing-docs", fmt.Sprintf("%s/google-beta/services", tpgbLocalPath)}, nil) + if err != nil { + return nil, err + } + + fmt.Println("detect missing docs return: ") + fmt.Println(output) + + var missingDocs *MissingDocsSummary + if err = json.Unmarshal([]byte(output), &missingDocs); err != nil { + return nil, err + } + fmt.Printf("missingDocs = %v\n", *missingDocs) + return missingDocs, rnr.PopDir() +} + func formatDiffComment(data diffCommentData) (string, error) { tmpl, err := template.New("DIFF_COMMENT.md.tmpl").Parse(diffComment) if err != nil { diff --git a/.ci/magician/cmd/generate_comment_test.go b/.ci/magician/cmd/generate_comment_test.go index 17566c575ae7..a825afbe16f9 100644 --- a/.ci/magician/cmd/generate_comment_test.go +++ b/.ci/magician/cmd/generate_comment_test.go @@ -93,6 +93,7 @@ func TestExecGenerateComment(t *testing.T) { {"/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{"detect-missing-docs", "/mock/dir/tpgb/google-beta/services"}, map[string]string(nil)}, {"/mock/dir/magic-modules/tools/diff-processor", "bin/diff-processor", []string{"schema-diff"}, map[string]string(nil)}, }, } { @@ -240,6 +241,55 @@ func TestFormatDiffComment(t *testing.T) { "## Errors", }, }, + "missing docs are displayed": { + data: diffCommentData{ + MissingDocs: &MissingDocsSummary{ + Resource: []MissingDocInfo{ + { + Name: "resource-a", + FilePath: "website/docs/r/resource-a.html.markdown", + Fields: []string{"field-a", "field-b"}, + }, + { + Name: "resource-b", + FilePath: "website/docs/r/resource-b.html.markdown", + Fields: []string{"field-a", "field-b"}, + }, + }, + DataSource: []MissingDocInfo{ + { + Name: "resource-a", + FilePath: "website/docs/d/resource-a.html.markdown", + }, + { + Name: "resource-b", + FilePath: "website/docs/d/resource-b.html.markdown", + }, + }, + }, + }, + expectedStrings: []string{ + "## Diff report", + "## Missing doc report", + }, + }, + "missing docs should not be displayed": { + data: diffCommentData{ + MissingDocs: &MissingDocsSummary{ + Resource: []MissingDocInfo{}, + DataSource: []MissingDocInfo{}, + }, + }, + notExpectedStrings: []string{ + "## Missing doc report", + }, + }, + "missing docs should not be displayed when MissingDocs is nil": { + data: diffCommentData{}, + notExpectedStrings: []string{ + "## Missing doc report", + }, + }, } for tn, tc := range cases { diff --git a/.ci/magician/cmd/mock_runner_test.go b/.ci/magician/cmd/mock_runner_test.go index 4de3d64d6978..a2ec5704d212 100644 --- a/.ci/magician/cmd/mock_runner_test.go +++ b/.ci/magician/cmd/mock_runner_test.go @@ -72,6 +72,7 @@ func NewMockRunner() MockRunner { "/mock/dir/magic-modules/tools/diff-processor make [build] " + sortedEnvString(diffProcessorEnv): "", "/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/magic-modules/tools/diff-processor bin/diff-processor [detect-missing-docs /mock/dir/tpgb/google-beta/services] map[]": `{"Resource":[],"DataSource":[]}`, "/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[]": "", "/mock/dir/tfoics git [diff origin/auto-pr-123456-old origin/auto-pr-123456 --shortstat] map[]": "", diff --git a/.ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl b/.ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl index 18e14fb10edd..f50fdee4626a 100644 --- a/.ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl +++ b/.ci/magician/cmd/templates/DIFF_COMMENT.md.tmpl @@ -51,6 +51,27 @@ If you believe this detection to be incorrect please raise the concern with your An `override-missing-service-label` label can be added to allow merging. {{end}} +{{- if and (.MissingDocs) (or .MissingDocs.Resource .MissingDocs.DataSource) }} +## Missing doc report (experimental) + +{{ if .MissingDocs.Resource }} +The following resources have fields missing in documents. +{{ range $inx, $missingDocInfo := .MissingDocs.Resource }} +- `{{ $missingDocInfo.Name }}` + - Expected Document Path: `{{ $missingDocInfo.FilePath }}` + - Fields: `{{ $missingDocInfo.Fields }}` +{{- end }} +{{- end }} + +{{ if .MissingDocs.DataSource }} +The following data sources are missing documents: +{{ range $inx, $missingDocInfo := .MissingDocs.DataSource }} +- `{{ $missingDocInfo.Name }}`, expect file `{{ $missingDocInfo.FilePath }}` to exist. +{{- end }} +{{- end }} + +{{- end }} + {{- $errorsLength := len .Errors}} {{- if gt $errorsLength 0}} ## Errors diff --git a/mmv1/third_party/terraform/services/apigee/resource_apigee_api.go b/mmv1/third_party/terraform/services/apigee/resource_apigee_api.go index 57375e59f2ae..e53dc9c8381a 100644 --- a/mmv1/third_party/terraform/services/apigee/resource_apigee_api.go +++ b/mmv1/third_party/terraform/services/apigee/resource_apigee_api.go @@ -132,6 +132,11 @@ func ResourceApigeeApi() *schema.Resource { return true }, }, + "dummy_field": { + Type: schema.TypeString, + Computed: true, + Description: `new field to test missing doc detector`, + }, }, UseJSONNumber: true, } diff --git a/tools/diff-processor/detector/detector.go b/tools/diff-processor/detector/detector.go index 92787befa4be..018d332d4802 100644 --- a/tools/diff-processor/detector/detector.go +++ b/tools/diff-processor/detector/detector.go @@ -166,15 +166,16 @@ func suggestedTest(resourceName string, untested []string) string { // DetectMissingDocs detect new fields that are missing docs given the schema diffs. // Return a map of resource names to missing doc info. +// It parses the document to see if the field is present within the resource document file, +// and is therefore heavily reliant on the document being written in the expected format. +// Should avoid printing to stdout since the output will be consumed in generate_comment.go. func DetectMissingDocs(schemaDiff diff.SchemaDiff, repoPath string) (map[string]MissingDocDetails, error) { ret := make(map[string]MissingDocDetails) for resource, resourceDiff := range schemaDiff { fieldsInDoc := make(map[string]bool) docFilePath, err := resourceToDocFile(resource, repoPath) - if err != nil { - fmt.Printf("Warning: %s.\n", err) - } else { + if err == nil { content, err := os.ReadFile(docFilePath) if err != nil { return nil, fmt.Errorf("failed to read resource doc %s: %w", docFilePath, err) @@ -218,6 +219,10 @@ func DetectMissingDocs(schemaDiff diff.SchemaDiff, repoPath string) (map[string] return ret, nil } +// DetectMissingDocsForDatasource detect new fields that are missing docs given the schema diffs. +// Return a map of resource names to missing doc info. +// It only checks whether the data source doc file exists. +// Should avoid printing to stdout since the output will be consumed in generate_comment.go. func DetectMissingDocsForDatasource(schemaDiff diff.SchemaDiff, repoPath string) (map[string]MissingDocDetails, error) { ret := make(map[string]MissingDocDetails) for resource, resourceDiff := range schemaDiff {