Skip to content

Commit

Permalink
chore(ci): return jsonify breaking change list (#10506)
Browse files Browse the repository at this point in the history
  • Loading branch information
iyabchen committed May 16, 2024
1 parent 9601417 commit c70a412
Show file tree
Hide file tree
Showing 12 changed files with 272 additions and 108 deletions.
4 changes: 2 additions & 2 deletions .ci/magician/cmd/DIFF_COMMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Your PR generated some diffs in downstreams - here they are.
The following breaking change(s) were detected within your pull request.

{{- range .BreakingChanges}}
- {{.}}{{end}}
- {{.Message}} - [reference]({{.DocumentationReference}}){{end}}

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a [major release](https://www.terraform.io/plugin/sdkv2/best-practices/versioning#example-major-number-increments) window.
Expand All @@ -33,4 +33,4 @@ An `override-breaking-change` label can be added to allow merging.
{{- range .Errors}}
- {{.}}{{end}}
{{end}}
{{- end -}}
{{- end -}}
25 changes: 18 additions & 7 deletions .ci/magician/cmd/generate_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ type Diff struct {
ShortStat string
}

type BreakingChange struct {
Message string
DocumentationReference string
}

type Errors struct {
Title string
Errors []string
Expand All @@ -58,7 +63,7 @@ type Errors struct {
type diffCommentData struct {
PrNumber int
Diffs []Diff
BreakingChanges []string
BreakingChanges []BreakingChange
MissingTests string
Errors []Errors
}
Expand Down Expand Up @@ -231,7 +236,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,

// The breaking changes are unique across both provider versions
uniqueAffectedResources := map[string]struct{}{}
uniqueBreakingChanges := map[string]struct{}{}
uniqueBreakingChanges := map[string]BreakingChange{}
diffProcessorPath := filepath.Join(mmLocalPath, "tools", "diff-processor")
diffProcessorEnv := map[string]string{
"OLD_REF": oldBranch,
Expand Down Expand Up @@ -263,7 +268,7 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
errors[repo.Title] = append(errors[repo.Title], "The diff processor crashed while computing breaking changes. This is usually due to the downstream provider failing to compile.")
}
for _, breakingChange := range breakingChanges {
uniqueBreakingChanges[breakingChange] = struct{}{}
uniqueBreakingChanges[breakingChange.Message] = breakingChange
}

if repo.Name == "terraform-provider-google-beta" {
Expand All @@ -285,8 +290,10 @@ func execGenerateComment(prNumber int, ghTokenMagicModules, buildId, buildStep,
uniqueAffectedResources[resource] = struct{}{}
}
}
breakingChangesSlice := maps.Keys(uniqueBreakingChanges)
sort.Strings(breakingChangesSlice)
breakingChangesSlice := maps.Values(uniqueBreakingChanges)
sort.Slice(breakingChangesSlice, func(i, j int) bool {
return breakingChangesSlice[i].Message < breakingChangesSlice[j].Message
})
data.BreakingChanges = breakingChangesSlice

// Compute affected resources based on changed files
Expand Down Expand Up @@ -412,7 +419,7 @@ func buildDiffProcessor(diffProcessorPath, providerLocalPath string, env map[str
return rnr.PopDir()
}

func computeBreakingChanges(diffProcessorPath string, rnr ExecRunner) ([]string, error) {
func computeBreakingChanges(diffProcessorPath string, rnr ExecRunner) ([]BreakingChange, error) {
if err := rnr.PushDir(diffProcessorPath); err != nil {
return nil, err
}
Expand All @@ -425,7 +432,11 @@ func computeBreakingChanges(diffProcessorPath string, rnr ExecRunner) ([]string,
return nil, nil
}

return strings.Split(strings.TrimSuffix(output, "\n"), "\n"), rnr.PopDir()
var changes []BreakingChange
if err = json.Unmarshal([]byte(output), &changes); err != nil {
return nil, err
}
return changes, rnr.PopDir()
}

func changedSchemaResources(diffProcessorPath string, rnr ExecRunner) ([]string, error) {
Expand Down
14 changes: 10 additions & 4 deletions .ci/magician/cmd/generate_comment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,17 +188,23 @@ func TestFormatDiffComment(t *testing.T) {
},
"breaking changes are displayed": {
data: diffCommentData{
BreakingChanges: []string{
"Breaking change 1",
"Breaking change 2",
BreakingChanges: []BreakingChange{
{
Message: "Breaking change 1",
DocumentationReference: "doc1",
},
{
Message: "Breaking change 2",
DocumentationReference: "doc2",
},
},
},
expectedStrings: []string{
"## Diff report",
"## Breaking Change(s) Detected",
"major release",
"`override-breaking-change`",
"- Breaking change 1\n- Breaking change 2\n",
"- Breaking change 1 - [reference](doc1)\n- Breaking change 2 - [reference](doc2)\n",
},
notExpectedStrings: []string{
"generated some diffs",
Expand Down
13 changes: 7 additions & 6 deletions tools/diff-processor/cmd/breaking_changes.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package cmd

import (
"encoding/json"
"fmt"
newProvider "google/provider/new/google/provider"
oldProvider "google/provider/old/google/provider"

Expand Down Expand Up @@ -42,12 +44,11 @@ func newBreakingChangesCmd(rootOptions *rootOptions) *cobra.Command {
func (o *breakingChangesOptions) run() error {
schemaDiff := o.computeSchemaDiff()
breakingChanges := rules.ComputeBreakingChanges(schemaDiff)
sort.Strings(breakingChanges)
for _, breakingChange := range breakingChanges {
_, err := o.stdout.Write([]byte(breakingChange + "\n"))
if err != nil {
return err
}
sort.Slice(breakingChanges, func(i, j int) bool {
return breakingChanges[i].Message < breakingChanges[j].Message
})
if err := json.NewEncoder(o.stdout).Encode(breakingChanges); err != nil {
return fmt.Errorf("error encoding json: %w", err)
}
return nil
}
21 changes: 11 additions & 10 deletions tools/diff-processor/cmd/breaking_changes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@ package cmd

import (
"bytes"
"encoding/json"
"testing"

"github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/diff"
"github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/rules"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"strings"
"testing"
)

func TestBreakingChangesCmd(t *testing.T) {
Expand Down Expand Up @@ -91,15 +94,13 @@ func TestBreakingChangesCmd(t *testing.T) {
out := make([]byte, buf.Len())
buf.Read(out)

lines := strings.Split(string(out), "\n")
nonemptyLines := []string{}
for _, line := range lines {
if line != "" {
nonemptyLines = append(nonemptyLines, line)
}
var got []rules.BreakingChange
if err = json.Unmarshal(out, &got); err != nil {
t.Fatalf("Failed to unmarshall output: %s", err)
}
if len(nonemptyLines) != tc.expectedViolations {
t.Errorf("Unexpected number of violations. Want %d, got %d. Output: %s", tc.expectedViolations, len(nonemptyLines), out)

if len(got) != tc.expectedViolations {
t.Errorf("Unexpected number of violations. Want %d, got %d. Output: %s", tc.expectedViolations, len(got), out)
}
})
}
Expand Down
22 changes: 17 additions & 5 deletions tools/diff-processor/rules/breaking_changes.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,18 @@ import (
"github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/diff"
)

func ComputeBreakingChanges(schemaDiff diff.SchemaDiff) []string {
messages := []string{}
type BreakingChange struct {
Resource string
Field string
Message string
DocumentationReference string
RuleTemplate string
RuleDefinition string
RuleName string
}

func ComputeBreakingChanges(schemaDiff diff.SchemaDiff) []*BreakingChange {
var messages []*BreakingChange
for resource, resourceDiff := range schemaDiff {
for _, rule := range ResourceInventoryRules {
if rule.isRuleBreak(resourceDiff.ResourceConfig.Old, resourceDiff.ResourceConfig.New) {
Expand Down Expand Up @@ -38,11 +48,13 @@ func ComputeBreakingChanges(schemaDiff diff.SchemaDiff) []string {
fieldDiff.Old,
fieldDiff.New,
MessageContext{
Resource: resource,
Field: field,
Resource: resource,
Field: field,
definition: rule.definition,
name: rule.name,
},
)
if breakageMessage != "" {
if breakageMessage != nil {
messages = append(messages, breakageMessage)
}
}
Expand Down

0 comments on commit c70a412

Please sign in to comment.