document - a batch of fix for documents (hashicorp#31081)

Author: QixiaLu

internal/tools/document-lint/check/check_possible_value.go

@@ -6,10 +6,15 @@ package check
 import (
 	"fmt"
 	"log"
+	"os"
 	"path"
+	"path/filepath"
+	"sort"
 	"strings"
+	"sync"
 
 	"github.com/fatih/color"
+	"github.com/hashicorp/terraform-provider-azurerm/internal/tools/document-lint/md"
 	"github.com/hashicorp/terraform-provider-azurerm/internal/tools/document-lint/model"
 	"github.com/hashicorp/terraform-provider-azurerm/internal/tools/document-lint/schema"
 	"github.com/hashicorp/terraform-provider-azurerm/internal/tools/document-lint/util"
@@ -112,9 +117,62 @@ func (p possibleValueDiff) Fix(line string) (result string, err error) {
 
 var _ Checker = (*possibleValueDiff)(nil)
 
+// sortPossibleValues sorts possible values in a sensible order
+// For weekdays, it maintains chronological order (Monday -> Sunday)
+// For other values, it sorts alphabetically (case-insensitive)
+func sortPossibleValues(values []string) []string {
+	if len(values) <= 1 {
+		return values
+	}
+
+	// Check if all values are weekdays
+	weekdayOrder := map[string]int{
+		"monday":    1,
+		"tuesday":   2,
+		"wednesday": 3,
+		"thursday":  4,
+		"friday":    5,
+		"saturday":  6,
+		"sunday":    7,
+		"Monday":    1,
+		"Tuesday":   2,
+		"Wednesday": 3,
+		"Thursday":  4,
+		"Friday":    5,
+		"Saturday":  6,
+		"Sunday":    7,
+	}
+
+	allWeekdays := true
+	for _, v := range values {
+		if _, ok := weekdayOrder[v]; !ok {
+			allWeekdays = false
+			break
+		}
+	}
+
+	sorted := make([]string, len(values))
+	copy(sorted, values)
+
+	if allWeekdays {
+		sort.Slice(sorted, func(i, j int) bool {
+			return weekdayOrder[sorted[i]] < weekdayOrder[sorted[j]]
+		})
+	} else {
+		sort.Slice(sorted, func(i, j int) bool {
+			return strings.ToLower(sorted[i]) < strings.ToLower(sorted[j])
+		})
+	}
+
+	return sorted
+}
+
 func patchWantEnums(want []string) string {
-	res := make([]string, len(want))
-	for idx, val := range want {
+	// Sort the values before formatting
+	sorted := sortPossibleValues(want)
+
+	res := make([]string, len(sorted))
+	for idx, val := range sorted {
 		res[idx] = "`" + val + "`"
 	}
 	if len(res) == 1 {
@@ -154,12 +212,21 @@ func diffField(r *schema.Resource, mdField *model.Field, xPath []string) (res []
 
 	// if end property
 	if mdField.Subs == nil {
-		want := r.PossibleValues[fullPath]
+		wanted := r.PossibleValues[fullPath]
 		docVal := mdField.PossibleValues()
-		if missed, spare := SliceDiff(want, docVal, true); len(missed)+len(spare) > 0 {
-			if !mayExistsInDoc(mdField.Content, want) {
+		if missed, spare := SliceDiff(wanted, docVal, true); len(missed)+len(spare) > 0 {
+			// Check if this property has version changes before reporting error
+			// Skip possible value diff if this property has changes in new version upgrade.
+			// This is because it's not consistent whether possible values change should be documented or not when it's in the upgrade feature flag
+			if hasVersionChanges(r.ResourceType, fullPath) {
+				log.Printf("[SKIP] %s.%s: Skipping possible values validation due to version changes detected in upgrade guide (missed: %v, spare: %v)",
+					r.ResourceType, fullPath, missed, spare)
+				return
+			}
+
+			if !mayExistsInDoc(mdField.Content, wanted) {
 				base := newCheckBase(mdField.Line, fullPath, mdField)
-				res = append(res, newPossibleValueDiff(base, want, docVal, missed, spare))
+				res = append(res, newPossibleValueDiff(base, wanted, docVal, missed, spare))
 			}
 		}
 		return
@@ -220,3 +287,110 @@ func mayExistsInDoc(docLine string, want []string) bool {
 	}
 	return true
 }
+
+// hasVersionChanges checks if the field has version-related changes by parsing the upgrade guide
+func hasVersionChanges(resourceType, fieldPath string) bool {
+	upgradeGuideContent := getUpgradeGuideContent()
+	if upgradeGuideContent == "" {
+		return false
+	}
+
+	// Look for the resource section
+	resourceSectionPattern := fmt.Sprintf("### `%s`", resourceType)
+
+	// Find the resource section
+	lines := strings.Split(upgradeGuideContent, "\n")
+	resourceSectionStart := -1
+
+	for i, line := range lines {
+		if strings.Contains(line, resourceSectionPattern) {
+			resourceSectionStart = i
+			break
+		}
+	}
+
+	// Resource not found in upgrade guide
+	if resourceSectionStart == -1 {
+		return false
+	}
+
+	// Find the end of this resource section (next resource or major section)
+	resourceSectionEnd := len(lines)
+	for i := resourceSectionStart + 1; i < len(lines); i++ {
+		line := lines[i]
+		if strings.HasPrefix(line, "### `azurerm_") || strings.HasPrefix(line, "## ") {
+			resourceSectionEnd = i
+			break
+		}
+	}
+
+	// Get the content of this resource section
+	resourceSection := strings.Join(lines[resourceSectionStart:resourceSectionEnd], "\n")
+
+	// Check if the property is mentioned in this resource section
+	propertyPatterns := []string{
+		fmt.Sprintf("`%s`", fieldPath), // `property_name`
+		fmt.Sprintf(" %s ", fieldPath), // property_name with spaces
+		fmt.Sprintf(".%s ", fieldPath), // .property_name
+		fmt.Sprintf("`%s.", fieldPath), // `property_name.
+	}
+
+	// For nested properties like "site_config.remote_debugging_version", also check the last part
+	if strings.Contains(fieldPath, ".") {
+		parts := strings.Split(fieldPath, ".")
+		lastPart := parts[len(parts)-1]
+		propertyPatterns = append(propertyPatterns,
+			fmt.Sprintf("`%s`", lastPart), // `last_part`
+			fmt.Sprintf(" %s ", lastPart), // last_part with spaces
+		)
+	}
+
+	// Check if any pattern is found in the resource section
+	for _, pattern := range propertyPatterns {
+		if strings.Contains(resourceSection, pattern) {
+			return true
+		}
+	}
+
+	return false
+}
+
+var (
+	upgradeGuideContent  string
+	loadUpgradeGuideOnce sync.Once
+)
+
+// getUpgradeGuideContent loads and caches the upgrade guide content
+func getUpgradeGuideContent() string {
+	loadUpgradeGuideOnce.Do(func() {
+		docsDir := md.DocDir()
+
+		if upgradeFile := findUpgradeGuide(docsDir); upgradeFile != "" {
+			if content, err := os.ReadFile(upgradeFile); err == nil {
+				upgradeGuideContent = string(content)
+				log.Printf("Loaded upgrade guide from: %s", upgradeFile)
+				return
+			}
+		}
+
+		log.Printf("Warning: Could not find any *-upgrade-guide.html.markdown file in docs directory: %s", docsDir)
+		upgradeGuideContent = ""
+	})
+
+	return upgradeGuideContent
+}
+
+// findUpgradeGuide searches for upgrade guide files in the given directory
+func findUpgradeGuide(dir string) string {
+	if _, err := os.Stat(dir); os.IsNotExist(err) {
+		return ""
+	}
+
+	pattern := filepath.Join(dir, "*-upgrade-guide.html.markdown")
+	matches, err := filepath.Glob(pattern)
+	if err != nil || len(matches) == 0 {
+		return ""
+	}
+
+	return matches[0]
+}

internal/tools/document-lint/check/register.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 
 	"github.com/hashicorp/terraform-provider-azurerm/internal/provider"
+	"github.com/hashicorp/terraform-provider-azurerm/internal/tools/document-lint/schema"
 )
 
 type resource struct {
@@ -77,6 +78,13 @@ func AzurermAllResources(service, skipService string, resources, skipResources s
 			if shouldSKipResource(svc.ResourceType()) {
 				continue
 			}
+
+			// Skip deprecated resources, as some of these don't have documents
+			sch := schema.NewResource(svc, svc.ResourceType())
+			if sch.IsDeprecated() {
+				continue
+			}
+
 			res.resources = append(res.resources, resource{
 				name:   svc.ResourceType(),
 				schema: svc,
@@ -93,6 +101,13 @@ func AzurermAllResources(service, skipService string, resources, skipResources s
 			if shouldSKipResource(name) {
 				continue
 			}
+
+			// Skip deprecated resources, as some of these don't have documents
+			sch := schema.NewResource(svc, name)
+			if sch.IsDeprecated() {
+				continue
+			}
+
 			res.resources = append(res.resources, resource{
 				name:   name,
 				schema: svc,

internal/tools/document-lint/check/skip_config.go

@@ -16,6 +16,10 @@ var skipProps = []string{
 	"all.timezone",
 	"all.time_zone",
 	"all.time_zone_id",
+	// Skip Key Vault reference properties - these reference external resources and shouldn't have "Possible values" documented
+	"all.key_vault_secret_id",
+	"all.key_vault_key_id",
+	"all.key_vault_certificate_id",
 	"azurerm_nginx_deployment.identity.type", // there is a diff between real supported values and common identity schema
 	"azurerm_kubernetes_cluster.default_node_pool.os_sku",
 	"azurerm_kubernetes_cluster_node_pool.os_sku",

internal/tools/document-lint/md/doc_helper.go

@@ -80,3 +80,8 @@ func ResourceDir() string {
 	}
 	return docRDir
 }
+
+// DocDir returns the base documentation directory
+func DocDir() string {
+	return docDir()
+}

internal/tools/document-lint/md/normalize.go

@@ -246,31 +246,34 @@ func tryFixProp(line string) string {
 			}
 		}
 	}
-	// need a dash after property name
-	idx := strings.Index(line, "`")
-	if idx += strings.Index(line[idx+1:], "`") + 1; idx > 0 {
-		for idx2 := idx + 1; idx2 < len(line); idx2++ {
-			if line[idx2] == ' ' {
-				continue
-			}
-			if line[idx2] != '-' {
-				line2 := line[:idx+2]
-				if line[idx2-1] != ' ' {
-					line2 += " "
-				}
-				line2 += "- "
-				line2 += line[idx2:]
-				line = line2
-				break
-			} else {
-				// if line[idx2] == '-'  exists
-				if line[idx2-1] != ' ' {
-					line = line[:idx2] + " " + line[idx2:]
+	// Skip adding dashes for note sections
+	if !strings.HasPrefix(line, "~>") && !strings.HasPrefix(line, "->") && !strings.HasPrefix(line, "!>") {
+		// need a dash after property name
+		idx := strings.Index(line, "`")
+		if idx += strings.Index(line[idx+1:], "`") + 1; idx > 0 {
+			for idx2 := idx + 1; idx2 < len(line); idx2++ {
+				if line[idx2] == ' ' {
+					continue
 				}
-				if line[idx2+1] != ' ' {
-					line = line[:idx2+1] + " " + line[idx2+1:]
+				if line[idx2] != '-' {
+					line2 := line[:idx+2]
+					if line[idx2-1] != ' ' {
+						line2 += " "
+					}
+					line2 += "- "
+					line2 += line[idx2:]
+					line = line2
+					break
+				} else {
+					// if line[idx2] == '-'  exists
+					if line[idx2-1] != ' ' {
+						line = line[:idx2] + " " + line[idx2:]
+					}
+					if line[idx2+1] != ' ' {
+						line = line[:idx2+1] + " " + line[idx2+1:]
+					}
+					break
 				}
-				break
 			}
 		}
 	}

internal/tools/document-lint/md/parse_md.go

@@ -165,7 +165,7 @@ func newMarkFromString(content string, filepath string) *Mark {
 			}
 		case strings.HasPrefix(line, "```"):
 			result.addLineOrItem(idx, line, ItemExample)
-		case strings.HasPrefix(line, "->"), strings.HasPrefix(line, "~>"):
+		case strings.HasPrefix(line, "->"), strings.HasPrefix(line, "~>"), strings.HasPrefix(line, "!>"):
 			result.addItemWith(idx, line, ItemNote)
 		case isBlockHead(line):
 			result.addItemWith(idx, line, ItemBlockHead)
@@ -317,11 +317,48 @@ func (m *Mark) blockOfName(name string, parent string, pos model.PosType) (b *Bl
 	}
 
 	if len(res) > 1 {
-		msg = fmt.Sprintf("duplicate block exists as name `%s`", util.Blue(name))
+		// Check if these are actual duplicate block definitions or just shared references
+		// Look at the actual block definitions to see if they have different content
+		uniqueDefinitions := make(map[string]Block)
+		for _, block := range res {
+			// Create a key based on the block's actual content/structure
+			key := fmt.Sprintf("%s:%d", block.Name, len(block.Fields))
+			if existing, exists := uniqueDefinitions[key]; exists {
+				// Check if they're actually different blocks
+				if !blocksHaveSameDefinition(existing, block) {
+					msg = fmt.Sprintf("duplicate block exists as name `%s`", util.Blue(name))
+					break
+				}
+			} else {
+				uniqueDefinitions[key] = block
+			}
+		}
+		// If we only have one unique definition, it's just a shared block reference, not a duplicate
 	}
 	return &res[0], msg
 }
 
+// blocksHaveSameDefinition checks if two blocks have the same definition/content
+func blocksHaveSameDefinition(b1, b2 Block) bool {
+	// Blocks are considered the same if they have the same fields
+	if len(b1.Fields) != len(b2.Fields) {
+		return false
+	}
+
+	// Compare each field
+	for i, f1 := range b1.Fields {
+		if i >= len(b2.Fields) {
+			return false
+		}
+		f2 := b2.Fields[i]
+		if f1.Name != f2.Name || f1.Required != f2.Required {
+			return false
+		}
+	}
+
+	return true
+}
+
 // buildStruct build struct of blocks
 func (m *Mark) buildStruct() {
 	fillField := func(f *model.Field, parent string) {

internal/tools/document-lint/md/parse_md_test.go

@@ -22,7 +22,7 @@ func Test_unmarshalFile(t *testing.T) {
 		itemNum int
 		argsNum int
 	}{
-		{"key_vault.html.markdown", 64, 16},
+		{"key_vault.html.markdown", 65, 16},
 		{"media_transform.html.markdown", 270, 5},
 	}
 	for _, arg := range args {