Skip to content

Commit 43be0d6

Browse files
committed
release: simplify update_workflow.go using YAML library (Option 2)
This refactors update_workflow.go to use gopkg.in/yaml.v3 instead of manual line-by-line parsing, making the code significantly more maintainable and easier to understand. Changes: - Replaced ~100 lines of manual YAML parsing with structured YAML library operations - Added yaml.v3 dependency for better formatting preservation - Simplified addBranchToWorkflow logic by navigating the parsed YAML tree instead of manually tracking line numbers and indentation - Extracted sortBranches helper function to reduce duplication This addresses feedback that the original manual parsing logic was difficult to read and maintain. Epic: None Release note: None
1 parent 2d22177 commit 43be0d6

File tree

2 files changed

+113
-110
lines changed

2 files changed

+113
-110
lines changed

pkg/cmd/release/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ go_library(
2424
"@com_github_jordan_wright_email//:email",
2525
"@com_github_spf13_cobra//:cobra",
2626
"@in_gopkg_yaml_v2//:yaml_v2",
27+
"@in_gopkg_yaml_v3//:yaml_v3",
2728
],
2829
)
2930

pkg/cmd/release/update_workflow.go

Lines changed: 112 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,13 @@ package main
77

88
import (
99
"fmt"
10-
"log"
1110
"os"
1211
"slices"
1312
"strings"
1413

1514
"github.com/cockroachdb/version"
1615
"github.com/spf13/cobra"
16+
"gopkg.in/yaml.v3"
1717
)
1818

1919
const workflowFile = ".github/workflows/update_releases.yaml"
@@ -77,7 +77,7 @@ func findLatestReleaseBranch() (string, error) {
7777
// Add .0 for patch to make it a valid semantic version
7878
v, err := version.Parse("v" + versionStr + ".0")
7979
if err != nil {
80-
log.Printf("WARNING: cannot parse version from branch %s: %v", branch, err)
80+
fmt.Printf("WARNING: cannot parse version from branch %s: %v\n", branch, err)
8181
continue
8282
}
8383
versions = append(versions, branchVersion{branch, v})
@@ -96,35 +96,9 @@ func findLatestReleaseBranch() (string, error) {
9696
return versions[len(versions)-1].branch, nil
9797
}
9898

99-
// addBranchToWorkflow adds the specified branch to the workflow file if not already present.
100-
func addBranchToWorkflow(branch string) error {
101-
// Read the workflow file
102-
rawData, err := os.ReadFile(workflowFile)
103-
if err != nil {
104-
return fmt.Errorf("failed to read workflow file: %w", err)
105-
}
106-
107-
lines := strings.Split(string(rawData), "\n")
108-
109-
// Find the branch section and extract current branches
110-
currentBranches, branchStart, branchEnd, indent, err := parseBranchSection(lines)
111-
if err != nil {
112-
return err
113-
}
114-
115-
// Check if branch already exists
116-
for _, b := range currentBranches {
117-
if b == branch {
118-
fmt.Printf("Branch %s is already in the workflow file\n", branch)
119-
return nil
120-
}
121-
}
122-
123-
// Add the new branch
124-
currentBranches = append(currentBranches, branch)
125-
126-
// Sort branches: master first, then release branches in version order
127-
slices.SortFunc(currentBranches, func(a, b string) int {
99+
// sortBranches sorts branches with master first, then release branches in version order.
100+
func sortBranches(branches []string) {
101+
slices.SortFunc(branches, func(a, b string) int {
128102
if a == "master" {
129103
return -1
130104
}
@@ -146,109 +120,137 @@ func addBranchToWorkflow(branch string) error {
146120

147121
return va.Compare(vb)
148122
})
123+
}
124+
125+
// addBranchToWorkflow adds the specified branch to the workflow file if not already present.
126+
func addBranchToWorkflow(branch string) error {
127+
// Read and parse the workflow file
128+
data, err := os.ReadFile(workflowFile)
129+
if err != nil {
130+
return fmt.Errorf("failed to read workflow file: %w", err)
131+
}
149132

150-
// Write the updated file
151-
if err := writeBranchSection(lines, currentBranches, branchStart, branchEnd, indent); err != nil {
133+
var workflow yaml.Node
134+
if err := yaml.Unmarshal(data, &workflow); err != nil {
135+
return fmt.Errorf("failed to parse workflow YAML: %w", err)
136+
}
137+
138+
// Navigate to jobs.update-crdb-releases-yaml.strategy.matrix.branch
139+
branchNode, err := findBranchNode(&workflow)
140+
if err != nil {
152141
return err
153142
}
154143

144+
// Extract current branches
145+
var currentBranches []string
146+
for _, item := range branchNode.Content {
147+
if item.Kind == yaml.ScalarNode {
148+
currentBranches = append(currentBranches, item.Value)
149+
}
150+
}
151+
152+
// Check if branch already exists
153+
if slices.Contains(currentBranches, branch) {
154+
fmt.Printf("Branch %s is already in the workflow file\n", branch)
155+
return nil
156+
}
157+
158+
// Add the new branch and sort
159+
currentBranches = append(currentBranches, branch)
160+
sortBranches(currentBranches)
161+
162+
// Update the YAML node with sorted branches
163+
branchNode.Content = make([]*yaml.Node, 0, len(currentBranches))
164+
for _, b := range currentBranches {
165+
branchNode.Content = append(branchNode.Content, &yaml.Node{
166+
Kind: yaml.ScalarNode,
167+
Style: yaml.DoubleQuotedStyle,
168+
Value: b,
169+
})
170+
}
171+
172+
// Marshal back to YAML
173+
var buf strings.Builder
174+
encoder := yaml.NewEncoder(&buf)
175+
encoder.SetIndent(2)
176+
if err := encoder.Encode(&workflow); err != nil {
177+
return fmt.Errorf("failed to encode YAML: %w", err)
178+
}
179+
encoder.Close()
180+
181+
// Write back to file
182+
if err := os.WriteFile(workflowFile, []byte(buf.String()), 0644); err != nil {
183+
return fmt.Errorf("failed to write workflow file: %w", err)
184+
}
185+
155186
fmt.Printf("Added branch %s to workflow file\n", branch)
156187
return nil
157188
}
158189

159-
// parseBranchSection parses the workflow file to find the branch matrix section.
160-
func parseBranchSection(
161-
lines []string,
162-
) (branches []string, start int, end int, indent string, err error) {
163-
start = -1
164-
end = -1
165-
166-
for i, line := range lines {
167-
// Look for "branch:" within the matrix section
168-
if strings.Contains(line, "branch:") && !strings.HasPrefix(strings.TrimSpace(line), "#") {
169-
start = i + 1
170-
// Determine indentation from the next line
171-
if i+1 < len(lines) {
172-
nextLine := lines[i+1]
173-
// Count leading spaces
174-
trimmed := strings.TrimLeft(nextLine, " ")
175-
indent = strings.Repeat(" ", len(nextLine)-len(trimmed))
176-
}
177-
} else if start != -1 && end == -1 {
178-
// Check if this line is still part of branch list
179-
trimmed := strings.TrimSpace(line)
180-
if trimmed == "" {
181-
// Empty line, continue
182-
continue
183-
}
184-
if !strings.HasPrefix(trimmed, "-") {
185-
// Found the end of the branch list
186-
end = i
187-
break
188-
} else {
189-
// Extract branch name from line like '- "release-25.4"'
190-
// Remove leading "- " and quotes
191-
branchLine := strings.TrimSpace(trimmed)
192-
branchLine = strings.TrimPrefix(branchLine, "- ")
193-
branchLine = strings.Trim(branchLine, "\"")
194-
branches = append(branches, branchLine)
195-
}
196-
}
197-
}
190+
// findBranchNode navigates the YAML tree to find the branch array node.
191+
func findBranchNode(root *yaml.Node) (*yaml.Node, error) {
192+
// Navigate: root -> jobs (map key) -> jobs (map value) ->
193+
// update-crdb-releases-yaml (key) -> update-crdb-releases-yaml (value) ->
194+
// strategy (key) -> strategy (value) -> matrix (key) -> matrix (value) ->
195+
// branch (key) -> branch (value - sequence)
198196

199-
if start == -1 {
200-
return nil, 0, 0, "", fmt.Errorf("branch section not found in workflow file")
197+
if root.Kind != yaml.DocumentNode || len(root.Content) == 0 {
198+
return nil, fmt.Errorf("invalid YAML structure: expected document node")
201199
}
202200

203-
// If we reached end of file, set end to len(lines)
204-
if end == -1 {
205-
end = len(lines)
201+
topMap := root.Content[0]
202+
if topMap.Kind != yaml.MappingNode {
203+
return nil, fmt.Errorf("invalid YAML structure: expected mapping at top level")
206204
}
207205

208-
return branches, start, end, indent, nil
209-
}
206+
// Find "jobs" key
207+
jobsNode := findMapValue(topMap, "jobs")
208+
if jobsNode == nil {
209+
return nil, fmt.Errorf("'jobs' key not found in workflow")
210+
}
210211

211-
// writeBranchSection writes the updated branch list back to the workflow file.
212-
func writeBranchSection(
213-
lines []string, branches []string, start int, end int, indent string,
214-
) error {
215-
// Build new lines
216-
var newLines []string
217-
newLines = append(newLines, lines[:start]...)
212+
// Find "update-crdb-releases-yaml" key
213+
jobNode := findMapValue(jobsNode, "update-crdb-releases-yaml")
214+
if jobNode == nil {
215+
return nil, fmt.Errorf("'update-crdb-releases-yaml' job not found")
216+
}
218217

219-
// Add branch lines
220-
for _, branch := range branches {
221-
newLines = append(newLines, fmt.Sprintf("%s- %q", indent, branch))
218+
// Find "strategy" key
219+
strategyNode := findMapValue(jobNode, "strategy")
220+
if strategyNode == nil {
221+
return nil, fmt.Errorf("'strategy' key not found in job")
222222
}
223223

224-
// Add remaining lines
225-
newLines = append(newLines, lines[end:]...)
224+
// Find "matrix" key
225+
matrixNode := findMapValue(strategyNode, "matrix")
226+
if matrixNode == nil {
227+
return nil, fmt.Errorf("'matrix' key not found in strategy")
228+
}
226229

227-
// Write back to file using atomic write pattern
228-
// Create temp file in the same directory as target to avoid cross-device link errors
229-
dir := ".github/workflows"
230-
f, err := os.CreateTemp(dir, "update_releases_*.yaml")
231-
if err != nil {
232-
return fmt.Errorf("could not create temporary file: %w", err)
230+
// Find "branch" key
231+
branchNode := findMapValue(matrixNode, "branch")
232+
if branchNode == nil {
233+
return nil, fmt.Errorf("'branch' key not found in matrix")
233234
}
234-
tmpName := f.Name()
235-
defer func() {
236-
if err != nil {
237-
_ = os.Remove(tmpName)
238-
}
239-
}()
240235

241-
if _, err = f.WriteString(strings.Join(newLines, "\n")); err != nil {
242-
f.Close()
243-
return fmt.Errorf("error writing to temp file: %w", err)
236+
if branchNode.Kind != yaml.SequenceNode {
237+
return nil, fmt.Errorf("'branch' value is not a sequence")
244238
}
245239

246-
if err = f.Close(); err != nil {
247-
return fmt.Errorf("error closing temp file: %w", err)
240+
return branchNode, nil
241+
}
242+
243+
// findMapValue finds a value in a mapping node by key.
244+
func findMapValue(mapNode *yaml.Node, key string) *yaml.Node {
245+
if mapNode.Kind != yaml.MappingNode {
246+
return nil
248247
}
249248

250-
if err = os.Rename(tmpName, workflowFile); err != nil {
251-
return fmt.Errorf("error moving file to final destination: %w", err)
249+
// Mapping nodes have alternating key-value pairs in Content
250+
for i := 0; i < len(mapNode.Content); i += 2 {
251+
if mapNode.Content[i].Value == key {
252+
return mapNode.Content[i+1]
253+
}
252254
}
253255

254256
return nil

0 commit comments

Comments
 (0)