From 141558502d6261a6e23fbccb01b2b97017f660fb Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 12 Nov 2024 19:48:39 +0000 Subject: [PATCH 1/3] CLOUDP-279336: Avoid that squashing overrides hideFromChangelog --- tools/cli/internal/changelog/outputfilter/squash.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/cli/internal/changelog/outputfilter/squash.go b/tools/cli/internal/changelog/outputfilter/squash.go index 75c0c6b635..efabfbddf3 100644 --- a/tools/cli/internal/changelog/outputfilter/squash.go +++ b/tools/cli/internal/changelog/outputfilter/squash.go @@ -135,11 +135,16 @@ func squashEntries(entries []*OasDiffEntry) ([]*OasDiffEntry, error) { entriesMap := newEntriesMapPerIDAndOperationID(entries) squashHandlers := newSquashHandlers() squashedEntries := []*OasDiffEntry{} + hidddenSquashedEntries := []*OasDiffEntry{} for _, entry := range entries { // if no squash handlers implemented for entry's code, // just append the entry to the result if _, ok := findHandler(entry.ID); !ok { + if entry.HideFromChangelog { + hidddenSquashedEntries = append(hidddenSquashedEntries, entry) + continue + } squashedEntries = append(squashedEntries, entry) continue } @@ -156,6 +161,7 @@ func squashEntries(entries []*OasDiffEntry) ([]*OasDiffEntry, error) { return nil, err } + squashedEntries = append(squashedEntries, hidddenSquashedEntries...) squashedEntries = append(squashedEntries, sortEntriesByDescription(entries)...) } From 52932c51fc1f03ae958ff429e301ed9db251af09 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 12 Nov 2024 21:12:21 +0000 Subject: [PATCH 2/3] Separate squashing of hidden and non-hidden entries --- .../internal/changelog/outputfilter/squash.go | 46 +++- .../changelog/outputfilter/squash_test.go | 244 +++++++++++++++++- 2 files changed, 271 insertions(+), 19 deletions(-) diff --git a/tools/cli/internal/changelog/outputfilter/squash.go b/tools/cli/internal/changelog/outputfilter/squash.go index efabfbddf3..1f28668b0d 100644 --- a/tools/cli/internal/changelog/outputfilter/squash.go +++ b/tools/cli/internal/changelog/outputfilter/squash.go @@ -112,44 +112,70 @@ func newSquashHandlers() []handler { } // EntryMappings groups entries by ID and then by OperationID and returns a Map[changeCode, Map[operationId, List[oasdiffEntry]]] -func newEntriesMapPerIDAndOperationID(entries []*OasDiffEntry) map[string]map[string][]*OasDiffEntry { - result := make(map[string]map[string][]*OasDiffEntry) +func newEntriesMapPerIDAndOperationID(entries []*OasDiffEntry) (result, hiddenResult map[string]map[string][]*OasDiffEntry) { + result = make(map[string]map[string][]*OasDiffEntry) + hiddenResult = make(map[string]map[string][]*OasDiffEntry) for _, entry := range entries { code := entry.ID operationID := entry.OperationID + hidden := entry.HideFromChangelog // Ensure the code map exists if _, exists := result[code]; !exists { result[code] = make(map[string][]*OasDiffEntry) } + if _, exists := hiddenResult[code]; !exists { + hiddenResult[code] = make(map[string][]*OasDiffEntry) + } + + if hidden { + // Append the entry to the appropriate operationID slice + hiddenResult[code][operationID] = append(hiddenResult[code][operationID], entry) + continue + } + // Append the entry to the appropriate operationID slice result[code][operationID] = append(result[code][operationID], entry) } - return result + return result, hiddenResult } func squashEntries(entries []*OasDiffEntry) ([]*OasDiffEntry, error) { - entriesMap := newEntriesMapPerIDAndOperationID(entries) + entriesByIDandOperationID, hiddenEntriesByIDandOperationID := newEntriesMapPerIDAndOperationID(entries) + squashHandlers := newSquashHandlers() squashedEntries := []*OasDiffEntry{} - hidddenSquashedEntries := []*OasDiffEntry{} for _, entry := range entries { // if no squash handlers implemented for entry's code, // just append the entry to the result if _, ok := findHandler(entry.ID); !ok { - if entry.HideFromChangelog { - hidddenSquashedEntries = append(hidddenSquashedEntries, entry) - continue - } squashedEntries = append(squashedEntries, entry) continue } } + squashedEntriesNotHidden, err := appplySquashHandlerToMap(squashHandlers, entriesByIDandOperationID) + if err != nil { + return nil, err + } + + squashedEntriesHidden, err := appplySquashHandlerToMap(squashHandlers, hiddenEntriesByIDandOperationID) + if err != nil { + return nil, err + } + + squashedEntries = append(squashedEntries, squashedEntriesNotHidden...) + squashedEntries = append(squashedEntries, squashedEntriesHidden...) + + return squashedEntries, nil +} + +func appplySquashHandlerToMap(squashHandlers []handler, entriesMap map[string]map[string][]*OasDiffEntry) ([]*OasDiffEntry, error) { + squashedEntries := []*OasDiffEntry{} for _, handler := range squashHandlers { entryMapPerOperationID, ok := entriesMap[handler.id] if !ok { @@ -161,10 +187,8 @@ func squashEntries(entries []*OasDiffEntry) ([]*OasDiffEntry, error) { return nil, err } - squashedEntries = append(squashedEntries, hidddenSquashedEntries...) squashedEntries = append(squashedEntries, sortEntriesByDescription(entries)...) } - return squashedEntries, nil } diff --git a/tools/cli/internal/changelog/outputfilter/squash_test.go b/tools/cli/internal/changelog/outputfilter/squash_test.go index 147ab6c451..421d2de317 100644 --- a/tools/cli/internal/changelog/outputfilter/squash_test.go +++ b/tools/cli/internal/changelog/outputfilter/squash_test.go @@ -2,6 +2,7 @@ package outputfilter import ( "reflect" + "sort" "testing" "github.com/stretchr/testify/require" @@ -9,14 +10,16 @@ import ( func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { testCases := []struct { - name string - entries []*OasDiffEntry - want map[string]map[string][]*OasDiffEntry + name string + entries []*OasDiffEntry + want map[string]map[string][]*OasDiffEntry + wantHidden map[string]map[string][]*OasDiffEntry }{ { - name: "Empty entries", - entries: []*OasDiffEntry{}, - want: map[string]map[string][]*OasDiffEntry{}, + name: "Empty entries", + entries: []*OasDiffEntry{}, + want: map[string]map[string][]*OasDiffEntry{}, + wantHidden: map[string]map[string][]*OasDiffEntry{}, }, { name: "Single entry", @@ -30,6 +33,8 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { }, }, }, + wantHidden: map[string]map[string][]*OasDiffEntry{ + "response-write-only-property-enum-value-added": {}}, }, { name: "Multiple entries with same ID", @@ -47,8 +52,9 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { }, }, }, + wantHidden: map[string]map[string][]*OasDiffEntry{ + "response-write-only-property-enum-value-added": {}}, }, - { name: "Multiple entries with same ID and OperationID", entries: []*OasDiffEntry{ @@ -63,6 +69,8 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { }, }, }, + wantHidden: map[string]map[string][]*OasDiffEntry{ + "response-write-only-property-enum-value-added": {}}, }, { name: "Multiple entries with different IDs", @@ -90,15 +98,70 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { }, }, }, + wantHidden: map[string]map[string][]*OasDiffEntry{ + "response-write-only-property-enum-value-added": {}, + "request-write-only-property-enum-value-added": {}, + }, + }, + { + name: "Hidden entries", + entries: []*OasDiffEntry{ + {ID: "response-write-only-property-enum-value-added", OperationID: "op1", HideFromChangelog: true}, + {ID: "response-write-only-property-enum-value-added", OperationID: "op2", HideFromChangelog: true}, + }, + want: map[string]map[string][]*OasDiffEntry{ + "response-write-only-property-enum-value-added": {}}, + wantHidden: map[string]map[string][]*OasDiffEntry{ + "response-write-only-property-enum-value-added": { + "op1": []*OasDiffEntry{ + {ID: "response-write-only-property-enum-value-added", OperationID: "op1", HideFromChangelog: true}, + }, + "op2": []*OasDiffEntry{ + {ID: "response-write-only-property-enum-value-added", OperationID: "op2", HideFromChangelog: true}, + }, + }, + }, + }, + { + name: "Mixed hidden and non-hidden entries", + entries: []*OasDiffEntry{ + {ID: "response-write-only-property-enum-value-added", OperationID: "op1"}, + {ID: "response-write-only-property-enum-value-added", OperationID: "op2", HideFromChangelog: true}, + {ID: "response-write-only-property-enum-value-added", OperationID: "op3"}, + {ID: "response-write-only-property-enum-value-added", OperationID: "op4", HideFromChangelog: true}, + }, + want: map[string]map[string][]*OasDiffEntry{ + "response-write-only-property-enum-value-added": { + "op1": []*OasDiffEntry{ + {ID: "response-write-only-property-enum-value-added", OperationID: "op1"}, + }, + "op3": []*OasDiffEntry{ + {ID: "response-write-only-property-enum-value-added", OperationID: "op3"}, + }, + }, + }, + wantHidden: map[string]map[string][]*OasDiffEntry{ + "response-write-only-property-enum-value-added": { + "op2": []*OasDiffEntry{ + {ID: "response-write-only-property-enum-value-added", OperationID: "op2", HideFromChangelog: true}, + }, + "op4": []*OasDiffEntry{ + {ID: "response-write-only-property-enum-value-added", OperationID: "op4", HideFromChangelog: true}, + }, + }, + }, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - got := newEntriesMapPerIDAndOperationID(tc.entries) + got, gotHidden := newEntriesMapPerIDAndOperationID(tc.entries) if !reflect.DeepEqual(got, tc.want) { t.Errorf("got %v, want %v", got, tc.want) } + if !reflect.DeepEqual(gotHidden, tc.wantHidden) { + t.Errorf("gotHidden %v, wantHidden %v", gotHidden, tc.wantHidden) + } }) } } @@ -238,3 +301,168 @@ func TestNewSquashMap(t *testing.T) { }) } } + +func TestSquashEntries(t *testing.T) { + testCases := []struct { + name string + entries []*OasDiffEntry + want []*OasDiffEntry + }{ + { + name: "Empty entries", + entries: []*OasDiffEntry{}, + want: []*OasDiffEntry{}, + }, + { + name: "Single entry", + entries: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1' enum value to the 'region' response write-only property", + }, + }, + want: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1' enum value to the 'region' response write-only property", + }, + }, + }, + { + name: "Multiple entries with same ID and OperationID", + entries: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1' enum value to the 'region' response write-only property", + }, + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM2' enum value to the 'region' response write-only property", + }, + }, + want: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1, ENUM2' enum values to the 'region' response write-only property", + }, + }, + }, + { + name: "Multiple entries with different IDs", + entries: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1' enum value to the 'region' response write-only property", + }, + { + ID: "request-write-only-property-enum-value-added", + OperationID: "op2", + Text: "added the new 'ENUM2' enum value to the 'region' request write-only property", + }, + }, + want: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1' enum value to the 'region' response write-only property", + }, + { + ID: "request-write-only-property-enum-value-added", + OperationID: "op2", + Text: "added the new 'ENUM2' enum value to the 'region' request write-only property", + }, + }, + }, + { + name: "Hidden entries", + entries: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1' enum value to the 'region' response write-only property", + HideFromChangelog: true, + }, + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM2' enum value to the 'region' response write-only property", + HideFromChangelog: true, + }, + }, + want: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1, ENUM2' enum values to the 'region' response write-only property", + HideFromChangelog: true, + }, + }, + }, + { + name: "Mixed hidden and non-hidden entries", + entries: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1' enum value to the 'region' response write-only property", + }, + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM2' enum value to the 'region' response write-only property", + HideFromChangelog: true, + }, + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM3' enum value to the 'region' response write-only property", + }, + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM4' enum value to the 'region' response write-only property", + HideFromChangelog: true, + }, + }, + want: []*OasDiffEntry{ + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM1, ENUM3' enum values to the 'region' response write-only property", + }, + { + ID: "response-write-only-property-enum-value-added", + OperationID: "op1", + Text: "added the new 'ENUM2, ENUM4' enum values to the 'region' response write-only property", + HideFromChangelog: true, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, err := squashEntries(tc.entries) + require.NoError(t, err) + sortEntries(got) + sortEntries(tc.want) + require.Equal(t, tc.want, got) + }) + } +} + +// sortEntries sorts the entries by their ID and OperationID. +func sortEntries(entries []*OasDiffEntry) { + sort.SliceStable(entries, func(i, j int) bool { + if entries[i].ID != entries[j].ID { + return entries[i].ID < entries[j].ID + } + return entries[i].OperationID < entries[j].OperationID + }) +} From cfbf18cbf5a865ceba89f6c22118e90b4eb98003 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Wed, 13 Nov 2024 10:30:54 +0000 Subject: [PATCH 3/3] Address comments --- .../internal/changelog/outputfilter/squash.go | 28 +++++++++---------- .../changelog/outputfilter/squash_test.go | 17 ++++------- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/tools/cli/internal/changelog/outputfilter/squash.go b/tools/cli/internal/changelog/outputfilter/squash.go index 1f28668b0d..92bbba0cee 100644 --- a/tools/cli/internal/changelog/outputfilter/squash.go +++ b/tools/cli/internal/changelog/outputfilter/squash.go @@ -111,7 +111,8 @@ func newSquashHandlers() []handler { } } -// EntryMappings groups entries by ID and then by OperationID and returns a Map[changeCode, Map[operationId, List[oasdiffEntry]]] +// EntryMappings groups entries by ID and then by OperationID and returns two maps +// of type Map[changeCode, Map[operationId, List[oasdiffEntry]]], one for hidden squashed entries and one for visible squashed entries. func newEntriesMapPerIDAndOperationID(entries []*OasDiffEntry) (result, hiddenResult map[string]map[string][]*OasDiffEntry) { result = make(map[string]map[string][]*OasDiffEntry) hiddenResult = make(map[string]map[string][]*OasDiffEntry) @@ -121,28 +122,27 @@ func newEntriesMapPerIDAndOperationID(entries []*OasDiffEntry) (result, hiddenRe operationID := entry.OperationID hidden := entry.HideFromChangelog - // Ensure the code map exists - if _, exists := result[code]; !exists { - result[code] = make(map[string][]*OasDiffEntry) - } - - if _, exists := hiddenResult[code]; !exists { - hiddenResult[code] = make(map[string][]*OasDiffEntry) - } - if hidden { - // Append the entry to the appropriate operationID slice - hiddenResult[code][operationID] = append(hiddenResult[code][operationID], entry) + addToMap(hiddenResult, code, operationID, entry) continue } - // Append the entry to the appropriate operationID slice - result[code][operationID] = append(result[code][operationID], entry) + addToMap(result, code, operationID, entry) } return result, hiddenResult } +func addToMap(m map[string]map[string][]*OasDiffEntry, code, operationID string, entry *OasDiffEntry) { + // Ensure the code map exists + if _, exists := m[code]; !exists { + m[code] = make(map[string][]*OasDiffEntry) + } + + // Append the entry to the appropriate operationID slice + m[code][operationID] = append(m[code][operationID], entry) +} + func squashEntries(entries []*OasDiffEntry) ([]*OasDiffEntry, error) { entriesByIDandOperationID, hiddenEntriesByIDandOperationID := newEntriesMapPerIDAndOperationID(entries) diff --git a/tools/cli/internal/changelog/outputfilter/squash_test.go b/tools/cli/internal/changelog/outputfilter/squash_test.go index 421d2de317..b3ccb7bbcb 100644 --- a/tools/cli/internal/changelog/outputfilter/squash_test.go +++ b/tools/cli/internal/changelog/outputfilter/squash_test.go @@ -33,8 +33,7 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { }, }, }, - wantHidden: map[string]map[string][]*OasDiffEntry{ - "response-write-only-property-enum-value-added": {}}, + wantHidden: map[string]map[string][]*OasDiffEntry{}, }, { name: "Multiple entries with same ID", @@ -52,8 +51,7 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { }, }, }, - wantHidden: map[string]map[string][]*OasDiffEntry{ - "response-write-only-property-enum-value-added": {}}, + wantHidden: map[string]map[string][]*OasDiffEntry{}, }, { name: "Multiple entries with same ID and OperationID", @@ -69,8 +67,7 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { }, }, }, - wantHidden: map[string]map[string][]*OasDiffEntry{ - "response-write-only-property-enum-value-added": {}}, + wantHidden: map[string]map[string][]*OasDiffEntry{}, }, { name: "Multiple entries with different IDs", @@ -98,10 +95,7 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { }, }, }, - wantHidden: map[string]map[string][]*OasDiffEntry{ - "response-write-only-property-enum-value-added": {}, - "request-write-only-property-enum-value-added": {}, - }, + wantHidden: map[string]map[string][]*OasDiffEntry{}, }, { name: "Hidden entries", @@ -109,8 +103,7 @@ func TestNewEntriesMapPerIDAndOperationID(t *testing.T) { {ID: "response-write-only-property-enum-value-added", OperationID: "op1", HideFromChangelog: true}, {ID: "response-write-only-property-enum-value-added", OperationID: "op2", HideFromChangelog: true}, }, - want: map[string]map[string][]*OasDiffEntry{ - "response-write-only-property-enum-value-added": {}}, + want: map[string]map[string][]*OasDiffEntry{}, wantHidden: map[string]map[string][]*OasDiffEntry{ "response-write-only-property-enum-value-added": { "op1": []*OasDiffEntry{