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: config injection new config validation #49

Merged
merged 13 commits into from
Jun 23, 2023
2 changes: 1 addition & 1 deletion pkg/workflow_handler/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ type WorkflowsClient interface {
ConstructTemplates(workflowsBatch *common.WorkflowsBatch, configName string) ([]v1alpha1.Template, error)
ConstructSpec(templates []v1alpha1.Template, params []v1alpha1.Parameter, configName string) (*v1alpha1.WorkflowSpec, error)
CreateWorkflow(spec *v1alpha1.WorkflowSpec, workflowsBatch *common.WorkflowsBatch) (*v1alpha1.Workflow, error)
SetConfig(wf *v1alpha1.Workflow, spec *v1alpha1.WorkflowSpec) error
SelectConfig(workflowsBatch *common.WorkflowsBatch) (string, error)
Lint(wf *v1alpha1.Workflow) error
Submit(ctx *context.Context, wf *v1alpha1.Workflow) error
HandleWorkflowBatch(ctx *context.Context, workflowsBatch *common.WorkflowsBatch) error
Expand Down
46 changes: 36 additions & 10 deletions pkg/workflow_handler/workflows.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,38 @@ func (wfc *WorkflowsClientImpl) CreateWorkflow(spec *v1alpha1.WorkflowSpec, work
return workflow, nil
}

func (wfc *WorkflowsClientImpl) SetConfig(wf *v1alpha1.Workflow, spec *v1alpha1.WorkflowSpec) error {
//TODO implement me
panic("implement me")
func (wfc *WorkflowsClientImpl) SelectConfig(workflowsBatch *common.WorkflowsBatch) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function never returns err so you can get rid of it in the return params.

It always returns just a string

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 !

var configName string
ok := IsConfigExists(&wfc.cfg.WorkflowsConfig, "default")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified to:

If config != "" and exists
Use it
Else
Use default

Default will always exists so no need to check for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

default not always exists..
it there only if configure, if not, it will use the CRDs default values..

Copy link
Contributor

Choose a reason for hiding this comment

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

So what this function returns in case there is no default and no other config ?

Copy link
Contributor

@qadiludmer qadiludmer Jun 23, 2023

Choose a reason for hiding this comment

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

Looks weird that there is a default to the default

Copy link
Contributor

@qadiludmer qadiludmer Jun 23, 2023

Choose a reason for hiding this comment

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

I would not handle config name default as special case here but as regular config that user decided to call it "default" so he needs to explicitly mention he wants to use it

Copy link
Contributor

Choose a reason for hiding this comment

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

It will make the config selection logic more predictable - you get what you asked for if it's existing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the CRD's default is blank, wheres piper default is the DevOps configuration provided

if ok {
configName = "default"
log.Printf(
"%s config selected for workflow in repo: %s branch %s",
configName,
workflowsBatch.Payload.Repo,
workflowsBatch.Payload.Branch,
) // Info
}
if *workflowsBatch.Config != "" {
ok = IsConfigExists(&wfc.cfg.WorkflowsConfig, *workflowsBatch.Config)
if ok {
configName = *workflowsBatch.Config
log.Printf(
"%s config overwrited for workflow in repo: %s branch %s",
*workflowsBatch.Config,
workflowsBatch.Payload.Repo,
workflowsBatch.Payload.Branch,
) // Info
} else {
log.Printf(
"error in selecting config, staying with default config for repo %s branch %s",
workflowsBatch.Payload.Repo,
workflowsBatch.Payload.Branch,
) // Error
}
}

return configName, nil
}

func (wfc *WorkflowsClientImpl) Lint(wf *v1alpha1.Workflow) error {
Expand All @@ -129,13 +158,10 @@ func (wfc *WorkflowsClientImpl) Submit(ctx *context.Context, wf *v1alpha1.Workfl

func (wfc *WorkflowsClientImpl) HandleWorkflowBatch(ctx *context.Context, workflowsBatch *common.WorkflowsBatch) error {
var params []v1alpha1.Parameter
var configName string
_, ok := wfc.cfg.WorkflowsConfig.Configs["default"]
if ok {
configName = "default"
}
if *workflowsBatch.Config != "" {
configName = *workflowsBatch.Config

configName, err := wfc.SelectConfig(workflowsBatch)
if err != nil {
return err
}

templates, err := wfc.ConstructTemplates(workflowsBatch, configName)
Expand Down
97 changes: 97 additions & 0 deletions pkg/workflow_handler/workflows_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package workflow_handler

import (
"github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
"github.com/rookout/piper/pkg/common"
"github.com/rookout/piper/pkg/conf"
"github.com/rookout/piper/pkg/git_provider"
assertion "github.com/stretchr/testify/assert"
"testing"
)

func TestSelectConfig(t *testing.T) {
var wfc *conf.WorkflowsConfig

assert := assertion.New(t)
// Create a sample WorkflowsBatch object for testing
configName := "default"
workflowsBatch := &common.WorkflowsBatch{
Config: &configName, // Set the desired config name here
Payload: &git_provider.WebhookPayload{},
}

// Create a mock WorkflowsClientImpl object with necessary dependencies
wfc = &conf.WorkflowsConfig{Configs: map[string]*conf.ConfigInstance{
"default": {Spec: v1alpha1.WorkflowSpec{},
OnExit: []v1alpha1.DAGTask{}},
"config1": {Spec: v1alpha1.WorkflowSpec{},
OnExit: []v1alpha1.DAGTask{}},
}}

wfcImpl := &WorkflowsClientImpl{
cfg: &conf.GlobalConfig{
WorkflowsConfig: *wfc,
},
}

// Call the SelectConfig function
returnConfigName, err := wfcImpl.SelectConfig(workflowsBatch)

// Assert the expected output
assert.Nil(err)
assert.Equal("default", returnConfigName)

// Test case 2
configName = "config1"
workflowsBatch = &common.WorkflowsBatch{
Config: &configName, // Set the desired config name here
Payload: &git_provider.WebhookPayload{},
}

// Call the SelectConfig function
returnConfigName, err = wfcImpl.SelectConfig(workflowsBatch)

// Assert the expected output
assert.Nil(err)
assert.Equal("config1", returnConfigName)

// Test case 3 - selection of non-existing config when default config exists
configName = "notInConfigs"
workflowsBatch = &common.WorkflowsBatch{
Config: &configName, // Set the desired config name here
Payload: &git_provider.WebhookPayload{},
}

// Call the SelectConfig function
returnConfigName, err = wfcImpl.SelectConfig(workflowsBatch)

// Assert the expected output
assert.Nil(err)
assert.Equal("default", returnConfigName)

// Test case 4 - selection of non-existing config when default config not exists
configName = "notInConfig"
workflowsBatch = &common.WorkflowsBatch{
Config: &configName, // Set the desired config name here
Payload: &git_provider.WebhookPayload{},
}

wfc4 := &conf.WorkflowsConfig{Configs: map[string]*conf.ConfigInstance{
"config1": {Spec: v1alpha1.WorkflowSpec{},
OnExit: []v1alpha1.DAGTask{}},
}}

wfcImpl4 := &WorkflowsClientImpl{
cfg: &conf.GlobalConfig{
WorkflowsConfig: *wfc4,
},
}

// Call the SelectConfig function
returnConfigName, err = wfcImpl4.SelectConfig(workflowsBatch)

// Assert the expected output
assert.Nil(err)
assert.NotNil(returnConfigName)

}
6 changes: 6 additions & 0 deletions pkg/workflow_handler/workflows_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
"github.com/rookout/piper/pkg/conf"
"github.com/rookout/piper/pkg/git_provider"
"github.com/rookout/piper/pkg/utils"
"gopkg.in/yaml.v3"
Expand Down Expand Up @@ -63,3 +64,8 @@ func GetParameters(paramsFile *git_provider.CommitFile) ([]v1alpha1.Parameter, e
}
return params, nil
}

func IsConfigExists(cfg *conf.WorkflowsConfig, config string) bool {
_, ok := cfg.Configs[config]
return ok
}