Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(yaml): support collapsing of multiple anchor keys in step #391

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 69 additions & 2 deletions yaml/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
"gopkg.in/yaml.v3"

"github.com/go-vela/types/library"
Expand Down Expand Up @@ -327,6 +328,72 @@ func TestYaml_Build_UnmarshalYAML(t *testing.T) {
},
},
},
{
file: "testdata/merge_anchor_step.yml",
want: &Build{
Version: "1",
Metadata: Metadata{
Template: false,
Clone: nil,
Environment: []string{"steps", "services", "secrets"},
},
Services: ServiceSlice{
{
Name: "service-a",
Ports: []string{"5432:5432"},
Environment: raw.StringSliceMap{
"REGION": "dev",
},
Image: "postgres",
Pull: "not_present",
},
},
Steps: StepSlice{
{
Commands: raw.StringSlice{"echo alpha"},
Name: "alpha",
Image: "alpine:latest",
Pull: "not_present",
Ruleset: Ruleset{
If: Rules{
Event: []string{"push"},
},
Matcher: "filepath",
Operator: "and",
},
},
{
Commands: raw.StringSlice{"echo beta"},
Name: "beta",
Image: "alpine:latest",
Pull: "not_present",
Ruleset: Ruleset{
If: Rules{
Event: []string{"push"},
},
Matcher: "filepath",
Operator: "and",
},
},
{
Commands: raw.StringSlice{"echo gamma"},
Name: "gamma",
Image: "alpine:latest",
Pull: "not_present",
Environment: raw.StringSliceMap{
"REGION": "dev",
},
Ruleset: Ruleset{
If: Rules{
Event: []string{"push"},
},
Matcher: "filepath",
Operator: "and",
},
},
},
},
},
{
file: "testdata/build_anchor_stage.yml",
want: &Build{
Expand Down Expand Up @@ -613,8 +680,8 @@ func TestYaml_Build_UnmarshalYAML(t *testing.T) {
t.Errorf("UnmarshalYAML returned err: %v", err)
}

if !reflect.DeepEqual(got, test.want) {
t.Errorf("UnmarshalYAML is %v, want %v", got, test.want)
if diff := cmp.Diff(test.want, got); diff != "" {
t.Errorf("UnmarshalYAML mismatch (-want +got):\n%s", diff)
}
}
}
83 changes: 72 additions & 11 deletions yaml/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"fmt"
"strings"

"gopkg.in/yaml.v3"

"github.com/go-vela/types/constants"
"github.com/go-vela/types/pipeline"
"github.com/go-vela/types/raw"
Expand Down Expand Up @@ -58,21 +60,78 @@ func (s *ServiceSlice) ToPipeline() *pipeline.ContainerSlice {
// UnmarshalYAML implements the Unmarshaler interface for the ServiceSlice type.
//
//nolint:dupl // accepting duplicative code that exists in step.go as well
func (s *ServiceSlice) UnmarshalYAML(unmarshal func(interface{}) error) error {
func (s *ServiceSlice) UnmarshalYAML(v *yaml.Node) error {
// service slice should be sequence
if v.Kind != yaml.SequenceNode {
return fmt.Errorf("invalid yaml: expected sequence node for service slice")
}

// service slice we try unmarshalling to
serviceSlice := new([]*Service)

// attempt to unmarshal as a service slice type
err := unmarshal(serviceSlice)
if err != nil {
return err
}

// iterate through each element in the service slice
for _, service := range *serviceSlice {
// handle nil service to avoid panic
if service == nil {
return fmt.Errorf("invalid service with nil content found")
for _, st := range v.Content {
// make local var
tmpService := *st

// services are mapping nodes
if tmpService.Kind != yaml.MappingNode {
return fmt.Errorf("invalid yaml: expected map node for service")
}

// initialize anchor node -- will be nil if `<<` never used
var anchorKey *yaml.Node

// initialize anchor sets
anchorList := new([]*yaml.Node) // collect multiple anchor references
newContent := new([]*yaml.Node) // new service content
anchorSequence := new(yaml.Node) // final type that is appended to service contents

// iterate through map contents (key, value)
for i := 0; i < len(tmpService.Content); i += 2 {
key := tmpService.Content[i]
value := tmpService.Content[i+1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im assuming this cant panic? i couldnt get it to be nil while testing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the same spirit, the length is guaranteed to be an even number? (i think so?) :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I believe the map type check will always make this an even length


// check if key is an anchor reference
if strings.EqualFold(key.Value, "<<") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EqualFold() seems unnecessary here (and in the other location).

Copy link
Contributor

@plyr4 plyr4 Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the equalfold is necessary you could use a switch case comparison for some extra speed

switch key.Value {
case "<<":

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's a simple key.Value == "<<", no? there are no casing concerns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, switch is faster tho (jump tables) but its negligible (with over 1 billion iterations it measures like 3% faster)

// if this is first anchor, initialize key and value
if anchorKey == nil {
anchorKey = key
anchorSequence = value
}

// append value to anchor list
*anchorList = append(*anchorList, value)
} else {
*newContent = append(*newContent, key, value)
}
}

// overwrite content
tmpService.Content = *newContent

// if there is only one anchor key, use existing sequence
if len(*anchorList) == 1 {
tmpService.Content = append(tmpService.Content, anchorKey, anchorSequence)
}

// if there are multiple anchor keys, create a sequence using anchorList as the content
if len(*anchorList) > 1 {
anchorSequence = new(yaml.Node)
anchorSequence.Kind = yaml.SequenceNode
anchorSequence.Style = yaml.FlowStyle
anchorSequence.Tag = "!!seq"
anchorSequence.Content = *anchorList

tmpService.Content = append(tmpService.Content, anchorKey, anchorSequence)
}

// convert processed node to service type
service := new(Service)

err := tmpService.Decode(service)
if err != nil {
return err
}

// implicitly set `pull` field if empty
Expand All @@ -97,6 +156,8 @@ func (s *ServiceSlice) UnmarshalYAML(unmarshal func(interface{}) error) error {
if strings.EqualFold(service.Pull, "false") {
service.Pull = constants.PullNotPresent
}

*serviceSlice = append(*serviceSlice, service)
}

// overwrite existing ServiceSlice
Expand Down
85 changes: 73 additions & 12 deletions yaml/step.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"fmt"
"strings"

"gopkg.in/yaml.v3"

"github.com/go-vela/types/constants"
"github.com/go-vela/types/pipeline"
"github.com/go-vela/types/raw"
Expand Down Expand Up @@ -72,22 +74,79 @@ func (s *StepSlice) ToPipeline() *pipeline.ContainerSlice {

// UnmarshalYAML implements the Unmarshaler interface for the StepSlice type.
//
//nolint:dupl // accepting duplicative code that exits in service.go as well
func (s *StepSlice) UnmarshalYAML(unmarshal func(interface{}) error) error {
//nolint:dupl // ignore similarities with service unmarshal
func (s *StepSlice) UnmarshalYAML(v *yaml.Node) error {
// step slice should be sequence
if v.Kind != yaml.SequenceNode {
return fmt.Errorf("invalid yaml: expected sequence node for step slice")
}

// step slice we try unmarshalling to
stepSlice := new([]*Step)

// attempt to unmarshal as a step slice type
err := unmarshal(stepSlice)
if err != nil {
return err
}

// iterate through each element in the step slice
for _, step := range *stepSlice {
// handle nil step to avoid panic
if step == nil {
return fmt.Errorf("invalid step with nil content found")
for _, st := range v.Content {
// make local var
tmpStep := *st

// steps are mapping nodes
if tmpStep.Kind != yaml.MappingNode {
return fmt.Errorf("invalid yaml: expected map node for step")
}

// initialize anchor node -- will be nil if `<<` never used
var anchorKey *yaml.Node

// initialize anchor sets
anchorList := new([]*yaml.Node) // collect multiple anchor references
newContent := new([]*yaml.Node) // new step content
anchorSequence := new(yaml.Node) // final type that is appended to step contents

// iterate through map contents (key, value)
for i := 0; i < len(tmpStep.Content); i += 2 {
key := tmpStep.Content[i]
value := tmpStep.Content[i+1]

// check if key is an anchor reference
if strings.EqualFold(key.Value, "<<") {
// if this is first anchor, initialize key and value
if anchorKey == nil {
anchorKey = key
anchorSequence = value
}

// append value to anchor list
*anchorList = append(*anchorList, value)
} else {
*newContent = append(*newContent, key, value)
}
}

// overwrite content
tmpStep.Content = *newContent

// if there is only one anchor key, use existing sequence
if len(*anchorList) == 1 {
tmpStep.Content = append(tmpStep.Content, anchorKey, anchorSequence)
}

// if there are multiple anchor keys, create a sequence using anchorList as the content
if len(*anchorList) > 1 {
anchorSequence = new(yaml.Node)
anchorSequence.Kind = yaml.SequenceNode
anchorSequence.Style = yaml.FlowStyle
anchorSequence.Tag = "!!seq"
anchorSequence.Content = *anchorList

tmpStep.Content = append(tmpStep.Content, anchorKey, anchorSequence)
}

// convert processed node to step type
step := new(Step)

err := tmpStep.Decode(step)
if err != nil {
return err
}

// implicitly set `pull` field if empty
Expand All @@ -112,6 +171,8 @@ func (s *StepSlice) UnmarshalYAML(unmarshal func(interface{}) error) error {
if strings.EqualFold(step.Pull, "false") {
step.Pull = constants.PullNotPresent
}

*stepSlice = append(*stepSlice, step)
}

// overwrite existing StepSlice
Expand Down
46 changes: 46 additions & 0 deletions yaml/testdata/merge_anchor_step.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# test file that uses the non-standard multiple anchor keys in one step to test custom step unmarshaler

version: "1"

aliases:
images:
alpine: &alpine-image
image: alpine:latest
postgres: &pg-image
image: postgres

events:
push: &event-push
ruleset:
event:
- push
env:
dev-env: &dev-environment
environment:
REGION: dev

services:
- name: service-a
<<: *pg-image
<<: *dev-environment
ports:
- "5432:5432"

steps:
- name: alpha
<<: *alpine-image
<<: *event-push
commands:
- echo alpha

- name: beta
<<: [ *alpine-image, *event-push ]
commands:
- echo beta

- name: gamma
<<: *alpine-image
<<: *event-push
<<: *dev-environment
commands:
- echo gamma
Loading