Skip to content

Commit fcb9bb7

Browse files
erezrokaherezrokah
authored
chore(ci): Add comment with summary for changed tables (#5347)
 #### Summary Fixes #3557. First step for #2769. Example in #5375. I chose a flat list, sorted by tables, then for each table breaking changes come first. I think a flat list can make it easier to add changelog entries to the PR, but we can always update the format of the comment. I'll follow up next with a PR to fail renovate updates if there's a breaking change in a non major version (#2769). Another possible solution is to use the plugins `doc --format json` command, but that requires checking out the code from `main`, running the command for each plugin, then doing the same for the PR branch, then comparing the JSON files. > Possible improvement, detect order changes to PKs as breaking <!--
1 parent 2d7b3f7 commit fcb9bb7

7 files changed

Lines changed: 656 additions & 0 deletions

File tree

‎.github/workflows/docs_changes_summary.yml‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
name: Summarize changes to source plugins docs
2+
3+
on:
4+
# Using pull_request_target works on forked PRs too. This is safe since we don't checkout the PR code (we only use the diff)
5+
pull_request_target:
6+
branches:
7+
- main
8+
9+
jobs:
10+
doc-changes:
11+
defaults:
12+
run:
13+
working-directory: scripts/table_diff
14+
timeout-minutes: 15
15+
runs-on: ubuntu-latest
16+
steps:
17+
- name: Checkout
18+
uses: actions/checkout@v3
19+
- name: Get PR diff
20+
run: |
21+
curl -L ${{ github.event.pull_request.diff_url }} > pr.diff
22+
- name: Set up Go 1.x
23+
uses: actions/setup-go@v3
24+
with:
25+
go-version-file: scripts/table_diff/go.mod
26+
cache: true
27+
cache-dependency-path: scripts/table_diff/go.sum
28+
- name: Generate docs changes file
29+
run: |
30+
go run main.go pr.diff changes.json
31+
- uses: actions/github-script@v6
32+
name: Get doc changes string
33+
id: get-changes
34+
with:
35+
result-encoding: string
36+
script: |
37+
const { promises: fs } = require('fs')
38+
const changes = JSON.parse(await fs.readFile('scripts/table_diff/changes.json', 'utf8'))
39+
if (changes.length === 0) {
40+
console.log('No changes to docs')
41+
return ""
42+
}
43+
const changesList = changes.map(change => {
44+
const { breaking, text } = change
45+
if (breaking) {
46+
return `- :warning: BREAKING CHANGE: ${text}`
47+
}
48+
return `- ${text}`
49+
}).join('\n')
50+
return changesList
51+
- name: Find Comment
52+
uses: peter-evans/find-comment@f4499a714d59013c74a08789b48abe4b704364a0
53+
if: steps.get-changes.outputs.result != ''
54+
id: find-comment
55+
with:
56+
issue-number: ${{ github.event.pull_request.number }}
57+
comment-author: 'github-actions[bot]'
58+
body-includes: '### This PR has the following changes to source plugin(s) tables:'
59+
- name: Create or update comment
60+
uses: peter-evans/create-or-update-comment@5adcb0bb0f9fb3f95ef05400558bdb3f329ee808
61+
if: steps.get-changes.outputs.result != ''
62+
with:
63+
comment-id: ${{ steps.find-comment.outputs.comment-id }}
64+
issue-number: ${{ github.event.pull_request.number }}
65+
body: |
66+
### This PR has the following changes to source plugin(s) tables:
67+
68+
${{steps.get-changes.outputs.result}}
69+
edit-mode: replace

‎scripts/table_diff/changes/changes.go‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
package changes
2+
3+
import (
4+
"fmt"
5+
"path/filepath"
6+
"regexp"
7+
"sort"
8+
"strings"
9+
10+
"github.com/bluekeyes/go-gitdiff/gitdiff"
11+
)
12+
13+
var (
14+
columnRegex = regexp.MustCompile(`^\|(?P<name>.*)\|(?P<dataType>.*)\|`)
15+
)
16+
17+
type change struct {
18+
Text string `json:"text"`
19+
Breaking bool `json:"breaking"`
20+
}
21+
22+
func backtickStrings(strings ...string) []interface{} {
23+
backticked := make([]interface{}, len(strings))
24+
for i, s := range strings {
25+
backticked[i] = fmt.Sprintf("`%s`", s)
26+
}
27+
return backticked
28+
}
29+
30+
func parseColumnChange(line string) (name string, dataType string) {
31+
match := columnRegex.FindStringSubmatch(line)
32+
if match == nil {
33+
return "", ""
34+
}
35+
result := make(map[string]string)
36+
for i, name := range columnRegex.SubexpNames() {
37+
if i != 0 && name != "" {
38+
result[name] = match[i]
39+
}
40+
}
41+
return result["name"], result["dataType"]
42+
}
43+
44+
func getColumnChanges(file *gitdiff.File, table string) (changes []change) {
45+
addedColumns := make(map[string]string)
46+
deletedColumns := make(map[string]string)
47+
for _, fragment := range file.TextFragments {
48+
for _, line := range fragment.Lines {
49+
name, dataType := parseColumnChange(line.Line)
50+
if name == "" || dataType == "" {
51+
continue
52+
}
53+
switch line.Op {
54+
case gitdiff.OpAdd:
55+
addedColumns[name] = dataType
56+
case gitdiff.OpDelete:
57+
deletedColumns[name] = dataType
58+
}
59+
}
60+
}
61+
for deletedName, deletedDataType := range deletedColumns {
62+
if addedDataType, ok := addedColumns[deletedName]; ok {
63+
if addedDataType != deletedDataType {
64+
changes = append(changes, change{
65+
Text: fmt.Sprintf("Table %s: column type changed from %s to %s for %s", backtickStrings(table, deletedDataType, addedDataType, deletedName)...),
66+
Breaking: true,
67+
})
68+
} else {
69+
changes = append(changes, change{
70+
Text: fmt.Sprintf("Table %s: column order changed for %s", backtickStrings(table, deletedName)...),
71+
Breaking: false,
72+
})
73+
}
74+
} else {
75+
changes = append(changes, change{
76+
Text: fmt.Sprintf("Table %s: column removed %s from table", backtickStrings(table, deletedName)...),
77+
Breaking: true,
78+
})
79+
}
80+
}
81+
for addedName, addedDataType := range addedColumns {
82+
if _, ok := deletedColumns[addedName]; !ok {
83+
changes = append(changes, change{
84+
Text: fmt.Sprintf("Table %s: column added with name %s and type %s", backtickStrings(table, addedName, addedDataType)...),
85+
Breaking: false,
86+
})
87+
}
88+
}
89+
90+
sort.SliceStable(changes, func(i, j int) bool {
91+
iBreaking := changes[i].Breaking
92+
jBreaking := changes[j].Breaking
93+
if iBreaking && !jBreaking {
94+
return true
95+
}
96+
if !iBreaking && jBreaking {
97+
return false
98+
}
99+
return changes[i].Text < changes[j].Text
100+
})
101+
return changes
102+
}
103+
104+
func getFileChanges(file *gitdiff.File) (changes []change, err error) {
105+
oldTableName := strings.TrimSuffix(filepath.Base(file.OldName), filepath.Ext(file.OldName))
106+
newTableName := strings.TrimSuffix(filepath.Base(file.NewName), filepath.Ext(file.NewName))
107+
108+
switch {
109+
case file.IsDelete:
110+
changes = append(changes, change{
111+
Text: fmt.Sprintf("Table %s was removed", backtickStrings(oldTableName)...),
112+
Breaking: true,
113+
})
114+
case file.IsRename:
115+
changes = append(changes, change{
116+
Text: fmt.Sprintf("Table %s was renamed to %s", backtickStrings(oldTableName, newTableName)...),
117+
Breaking: true,
118+
})
119+
case file.IsNew:
120+
changes = append(changes, change{
121+
Text: fmt.Sprintf("Table %s was added", backtickStrings(newTableName)...),
122+
Breaking: false,
123+
})
124+
case file.IsCopy:
125+
return nil, fmt.Errorf("unhandled IsCopy table diff, %s -> %s", backtickStrings(oldTableName, newTableName)...)
126+
}
127+
128+
checkColumnChanges := !file.IsDelete && !file.IsNew
129+
// Don't report column changes for deleted or new tables to avoid noise
130+
if checkColumnChanges {
131+
changes = append(changes, getColumnChanges(file, newTableName)...)
132+
}
133+
134+
return changes, nil
135+
}
136+
137+
func GetChanges(files []*gitdiff.File) (changes []change, err error) {
138+
changes = make([]change, 0)
139+
for _, file := range files {
140+
fileChanges, err := getFileChanges(file)
141+
if err != nil {
142+
return nil, err
143+
}
144+
changes = append(changes, fileChanges...)
145+
}
146+
147+
return changes, nil
148+
}

‎scripts/table_diff/changes/changes_test.go‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
package changes
2+
3+
import (
4+
"os"
5+
"testing"
6+
7+
"github.com/bluekeyes/go-gitdiff/gitdiff"
8+
"github.com/stretchr/testify/require"
9+
)
10+
11+
func getDiff(t *testing.T, diffDataFile string) []*gitdiff.File {
12+
t.Helper()
13+
patch, err := os.Open(diffDataFile)
14+
if err != nil {
15+
t.Fatal(err)
16+
}
17+
18+
files, _, err := gitdiff.Parse(patch)
19+
if err != nil {
20+
t.Fatal(err)
21+
}
22+
return files
23+
}
24+
25+
func Test_parseColumnChange(t *testing.T) {
26+
type args struct {
27+
line string
28+
}
29+
tests := []struct {
30+
name string
31+
args args
32+
wantName string
33+
wantDataType string
34+
}{
35+
{name: "Should parse name and data type when change is a column", args: args{line: "|name|String|"}, wantName: "name", wantDataType: "String"},
36+
{name: "Should return empty strings when change is not a column", args: args{line: "# Table: azure_appservice_site_auth_settings"}, wantName: "", wantDataType: ""},
37+
}
38+
for _, tt := range tests {
39+
t.Run(tt.name, func(t *testing.T) {
40+
gotName, gotDataType := parseColumnChange(tt.args.line)
41+
require.Equal(t, tt.wantName, gotName)
42+
require.Equal(t, tt.wantDataType, gotDataType)
43+
})
44+
}
45+
}
46+
47+
func Test_getChanges(t *testing.T) {
48+
tests := []struct {
49+
name string
50+
diffDataFile string
51+
wantChanges []change
52+
wantErr bool
53+
}{
54+
{
55+
name: "Should return breaking changes",
56+
diffDataFile: "testdata/pr_4768_diff.txt",
57+
wantChanges: []change{
58+
{
59+
Text: "Table `azure_web_functions` was renamed to `azure_appservice_functions`",
60+
Breaking: true,
61+
},
62+
{
63+
Text: "Table `azure_appservice_functions`: column removed `web_app_id` from table",
64+
Breaking: true,
65+
},
66+
{
67+
Text: "Table `azure_appservice_functions`: column added with name `site_id` and type `String`",
68+
Breaking: false,
69+
},
70+
{
71+
Text: "Table `azure_appservice_functions`: column order changed for `function_app_id`",
72+
Breaking: false,
73+
},
74+
{
75+
Text: "Table `azure_appservice_functions`: column order changed for `href`",
76+
Breaking: false,
77+
},
78+
{
79+
Text: "Table `azure_appservice_functions`: column order changed for `kind`",
80+
Breaking: false,
81+
},
82+
{
83+
Text: "Table `azure_appservice_functions`: column order changed for `language`",
84+
Breaking: false,
85+
},
86+
{
87+
Text: "Table `azure_appservice_functions`: column order changed for `script_href`",
88+
Breaking: false,
89+
},
90+
{
91+
Text: "Table `azure_appservice_functions`: column order changed for `script_root_path_href`",
92+
Breaking: false,
93+
},
94+
{
95+
Text: "Table `azure_appservice_functions`: column order changed for `secrets_file_href`",
96+
Breaking: false,
97+
},
98+
{
99+
Text: "Table `azure_appservice_functions`: column order changed for `test_data_href`",
100+
Breaking: false,
101+
},
102+
{
103+
Text: "Table `azure_appservice_functions`: column order changed for `test_data`",
104+
Breaking: false,
105+
},
106+
{
107+
Text: "Table `azure_appservice_functions`: column order changed for `type`",
108+
Breaking: false,
109+
},
110+
{
111+
Text: "Table `azure_subscription_locations` was added",
112+
Breaking: false,
113+
},
114+
{
115+
Text: "Table `azure_resources_links` was renamed to `azure_subscription_tenants`",
116+
Breaking: true,
117+
},
118+
{
119+
Text: "Table `azure_subscription_tenants`: column removed `name` from table",
120+
Breaking: true,
121+
},
122+
{
123+
Text: "Table `azure_subscription_tenants`: column removed `properties_notes` from table",
124+
Breaking: true,
125+
},
126+
{
127+
Text: "Table `azure_subscription_tenants`: column removed `properties_source_id` from table",
128+
Breaking: true,
129+
},
130+
{
131+
Text: "Table `azure_subscription_tenants`: column removed `properties_target_id` from table",
132+
Breaking: true,
133+
},
134+
{
135+
Text: "Table `azure_subscription_tenants`: column added with name `tenant_id` and type `String`",
136+
Breaking: false,
137+
},
138+
{
139+
Text: "Table `azure_subscriptions`: column removed `managed_by_tenants` from table",
140+
Breaking: true,
141+
},
142+
{
143+
Text: "Table `azure_subscriptions`: column removed `tags` from table",
144+
Breaking: true,
145+
},
146+
{
147+
Text: "Table `azure_subscriptions`: column removed `tenant_id` from table",
148+
Breaking: true,
149+
},
150+
{
151+
Text: "Table `azure_subscriptions`: column added with name `subscription_id` and type `String`",
152+
Breaking: false,
153+
},
154+
{
155+
Text: "Table `azure_web_publishing_profiles` was removed",
156+
Breaking: true,
157+
},
158+
},
159+
wantErr: false,
160+
},
161+
}
162+
for _, tt := range tests {
163+
t.Run(tt.name, func(t *testing.T) {
164+
diff := getDiff(t, tt.diffDataFile)
165+
gotChanges, err := GetChanges(diff)
166+
if tt.wantErr {
167+
require.Error(t, err)
168+
}
169+
require.Equal(t, tt.wantChanges, gotChanges)
170+
})
171+
}
172+
}

0 commit comments

Comments
 (0)