-
Notifications
You must be signed in to change notification settings - Fork 13
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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] | ||
|
||
// check if key is an anchor reference | ||
if strings.EqualFold(key.Value, "<<") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EqualFold() seems unnecessary here (and in the other location). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 "<<": There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it's a simple There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
|
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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