drone / go-convert

Package convert provides tools for converting pipeline configuration files to the Drone format.
Apache License 2.0
9 stars 8 forks source link

increases test coverage for gitlab parsing tests and fixes issues. #143

Closed eoinmcafee00 closed 9 months ago

bradrydzewski commented 9 months ago

just an fyi am getting some test failures

--- FAIL: TestInheritKeys_Error (0.00s)
    go-convert/convert/gitlab/yaml/inherit_test.go:59: Expect error, got failed to unmarshal inherit keys
--- FAIL: TestJob (0.00s)
    go-convert/convert/gitlab/yaml/job_test.go:119: yaml: unmarshal errors:
          line 1: cannot unmarshal !!seq into yaml.Job
--- FAIL: TestJob_Error (0.00s)
    go-convert/convert/gitlab/yaml/job_test.go:132: Expect error, got yaml: unmarshal errors:
          line 1: cannot unmarshal !!int `123` into yaml.Job
bradrydzewski commented 9 months ago

another things I noticed is that the golden files don't produce a valid gitlab yaml

jobs:
    job:
        artifacts:
            paths:
                - binaries/
                - .config

This probably means we are missing a custom marshal function in one or more places. It is important that the golden file be a valid gitlab yaml to ensure we can accurately serialize. it also makes it easier for a third party (like myself) to read the tests and verify accuracy.

Note that anywhere we do a custom unmarshal, we should verify that it produces a correct golden file. If not we need to implement custom marshaling (you can search the codebase for MarshalYaml for examples)

bradrydzewski commented 9 months ago

I can't run the full test suite to verify due to golden file format, but I feel like we should be able to simplify the unmarshal logic below. Note that I also added a custom marshal function, so that we can marshal the pipeline back to a valid GitLab yaml without data loss.

package yaml

import (
    "strings"
)

type (
    // Pipeline defines a gitlab pipeline.
    Pipeline struct {
        Default      *Default
        Include      []*Include
        Image        *Image
        Jobs         map[string]*Job
        TemplateJobs map[string]*Job
        Stages       []string
        Variables    map[string]*Variable
        Workflow     *Workflow
    }

    // pipeline is a temporary structure to parse the pipeline.
    pipeline struct {
        Default   *Default             `yaml:"default,omitempty"`
        Include   []*Include           `yaml:"include,omitempty"`
        Image     *Image               `yaml:"image,omitempty"`
        Jobs      map[string]*Job      `yaml:"jobs,inline"`
        Stages    []string             `yaml:"stages,omitempty"`
        Variables map[string]*Variable `yaml:"variables,omitempty"`
        Workflow  *Workflow            `yaml:"workflow,omitempty"`
    }

    // Default defines global pipeline defaults.
    Default struct {
        After         Stringorslice `yaml:"after_script,omitempty"`
        Before        Stringorslice `yaml:"before_script,omitempty"`
        Artifacts     *Artifacts    `yaml:"artifacts,omitempty"`
        Cache         *Cache        `yaml:"cache,omitempty"`
        Image         *Image        `yaml:"image,omitempty"`
        Interruptible bool          `yaml:"interruptible,omitempty"`
        Retry         *Retry        `yaml:"retry,omitempty"`
        Services      []*Image      `yaml:"services,omitempty"`
        Tags          Stringorslice `yaml:"tags,omitempty"`
        Timeout       string        `yaml:"duration,omitempty"`
    }
)

func (p *Pipeline) UnmarshalYAML(unmarshal func(interface{}) error) error {
    out := new(pipeline)
    if err := unmarshal(&out); err != nil {
        return err
    }

    p.Default = out.Default
    p.Include = out.Include
    p.Image = out.Image
    p.Jobs = out.Jobs
    p.Stages = out.Stages
    p.Variables = out.Variables
    p.Workflow = out.Workflow
    p.TemplateJobs = map[string]*Job{}

    for k, v := range out.Jobs {
        if strings.HasPrefix(k, ".") {
            delete(p.Jobs, k)
            p.TemplateJobs[k] = v
        }
    }
    return nil
}

// MarshalYAML implements yaml marshalling.
func (p *Pipeline) MarshalYAML() (interface{}, error) {
    out := new(pipeline)
    out.Default = p.Default
    out.Include = p.Include
    out.Image = p.Image
    out.Jobs = p.Jobs
    out.Stages = p.Stages
    out.Variables = p.Variables
    out.Workflow = p.Workflow
    for k, v := range p.TemplateJobs {
        out.Jobs[k] = v
    }
    return out, nil
}
eoinmcafee00 commented 9 months ago

another things I noticed is that the golden files don't produce a valid gitlab yaml

jobs:
    job:
        artifacts:
            paths:
                - binaries/
                - .config

This probably means we are missing a custom marshal function in one or more places. It is important that the golden file be a valid gitlab yaml to ensure we can accurately serialize. it also makes it easier for a third party (like myself) to read the tests and verify accuracy.

Note that anywhere we do a custom unmarshal, we should verify that it produces a correct golden file. If not we need to implement custom marshaling (you can search the codebase for MarshalYaml for examples)

Fixed this.