From 4d9b08bd547b6dd642a6e197c25c5841ce22dffb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Barcarol=20Guimar=C3=A3es?= Date: Fri, 17 Apr 2020 10:34:10 +0000 Subject: [PATCH 1/6] ci-operator multi-stage: remove unnecessary field --- pkg/steps/multi_stage.go | 32 ++++++++++++++++++-------------- pkg/steps/multi_stage_test.go | 4 ++-- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/pkg/steps/multi_stage.go b/pkg/steps/multi_stage.go index 9a6ccf77fef..6153559edc9 100644 --- a/pkg/steps/multi_stage.go +++ b/pkg/steps/multi_stage.go @@ -49,9 +49,7 @@ type multiStageTestStep struct { profile api.ClusterProfile config *api.ReleaseBuildConfiguration // params exposes getters for variables created by other steps - params api.Parameters - // env holds all of the variables we need to expose to pods - env []coreapi.EnvVar + params api.Parameters podClient PodClient secretClient coreclientset.SecretsGetter saClient coreclientset.ServiceAccountsGetter @@ -124,6 +122,7 @@ func (s *multiStageTestStep) Run(ctx context.Context, dry bool) error { func (s *multiStageTestStep) run(ctx context.Context, dry bool) error { s.dry = dry + var env []coreapi.EnvVar if s.profile != "" { if !dry { secret := s.profileSecretName() @@ -131,17 +130,17 @@ func (s *multiStageTestStep) run(ctx context.Context, dry bool) error { return fmt.Errorf("could not find secret %q: %v", secret, err) } } - for _, env := range envForProfile { - val, err := s.params.Get(env) + for _, e := range envForProfile { + val, err := s.params.Get(e) if err != nil { return err } - s.env = append(s.env, coreapi.EnvVar{Name: env, Value: val}) + env = append(env, coreapi.EnvVar{Name: e, Value: val}) } if optionalOperator, err := resolveOptionalOperator(s.params); err != nil { return err } else if optionalOperator != nil { - s.env = append(s.env, optionalOperator.asEnv()...) + env = append(env, optionalOperator.asEnv()...) } } if err := s.createSecret(); err != nil { @@ -154,12 +153,12 @@ func (s *multiStageTestStep) run(ctx context.Context, dry bool) error { return fmt.Errorf("failed to create RBAC objects: %v", err) } var errs []error - if err := s.runSteps(ctx, s.pre, true); err != nil { + if err := s.runSteps(ctx, s.pre, env, true); err != nil { errs = append(errs, fmt.Errorf("%q pre steps failed: %v", s.name, err)) - } else if err := s.runSteps(ctx, s.test, true); err != nil { + } else if err := s.runSteps(ctx, s.test, env, true); err != nil { errs = append(errs, fmt.Errorf("%q test steps failed: %v", s.name, err)) } - if err := s.runSteps(context.Background(), s.post, false); err != nil { + if err := s.runSteps(context.Background(), s.post, env, false); err != nil { errs = append(errs, fmt.Errorf("%q post steps failed: %v", s.name, err)) } return utilerrors.NewAggregate(errs) @@ -307,8 +306,13 @@ func (s *multiStageTestStep) createCredentials() error { return nil } -func (s *multiStageTestStep) runSteps(ctx context.Context, steps []api.LiteralTestStep, shortCircuit bool) error { - pods, err := s.generatePods(steps) +func (s *multiStageTestStep) runSteps( + ctx context.Context, + steps []api.LiteralTestStep, + env []coreapi.EnvVar, + shortCircuit bool, +) error { + pods, err := s.generatePods(steps, env) if err != nil { return err } @@ -331,7 +335,7 @@ func (s *multiStageTestStep) runSteps(ctx context.Context, steps []api.LiteralTe return utilerrors.NewAggregate(errs) } -func (s *multiStageTestStep) generatePods(steps []api.LiteralTestStep) ([]coreapi.Pod, error) { +func (s *multiStageTestStep) generatePods(steps []api.LiteralTestStep, env []coreapi.EnvVar) ([]coreapi.Pod, error) { var ret []coreapi.Pod var errs []error for _, step := range steps { @@ -367,7 +371,7 @@ func (s *multiStageTestStep) generatePods(steps []api.LiteralTestStep) ([]coreap {Name: "JOB_NAME_SAFE", Value: strings.Replace(s.name, "_", "-", -1)}, {Name: "JOB_NAME_HASH", Value: s.jobSpec.JobNameHash()}, }...) - container.Env = append(container.Env, s.env...) + container.Env = append(container.Env, env...) if owner := s.jobSpec.Owner(); owner != nil { pod.OwnerReferences = append(pod.OwnerReferences, *owner) } diff --git a/pkg/steps/multi_stage_test.go b/pkg/steps/multi_stage_test.go index c820fe19b65..6c4c2dc8977 100644 --- a/pkg/steps/multi_stage_test.go +++ b/pkg/steps/multi_stage_test.go @@ -156,12 +156,12 @@ func TestGeneratePods(t *testing.T) { Namespace: "namespace", } step := newMultiStageTestStep(config.Tests[0], &config, nil, nil, nil, nil, nil, "artifact_dir", &jobSpec, nil) - step.env = []coreapi.EnvVar{ + env := []coreapi.EnvVar{ {Name: "RELEASE_IMAGE_INITIAL", Value: "release:initial"}, {Name: "RELEASE_IMAGE_LATEST", Value: "release:latest"}, {Name: "LEASED_RESOURCE", Value: "uuid"}, } - ret, err := step.generatePods(config.Tests[0].MultiStageTestConfigurationLiteral.Test) + ret, err := step.generatePods(config.Tests[0].MultiStageTestConfigurationLiteral.Test, env) if err != nil { t.Fatal(err) } From 62e7498d6667599dbefe651fe6a8e32f0e0d1dfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Barcarol=20Guimar=C3=A3es?= Date: Wed, 27 May 2020 11:53:35 +0000 Subject: [PATCH 2/6] ci-operator multi-stage: add step parameters Implementation of the execution side of the proposal described in: https://docs.google.com/document/d/14CLXQ4NgHNZHw_FGfmNsqxC1FGgK5qVb6uRc9XIafws --- pkg/api/types.go | 19 +++++++++ pkg/steps/multi_stage.go | 24 +++++++++-- pkg/steps/multi_stage_test.go | 75 +++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 4 deletions(-) diff --git a/pkg/api/types.go b/pkg/api/types.go index cc354f6877b..6360daca939 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -560,6 +560,18 @@ type LiteralTestStep struct { Resources ResourceRequirements `json:"resources,omitempty"` // Credentials defines the credentials we'll mount into this step. Credentials []CredentialReference `json:"credentials,omitempty"` + // Environment lists parameters that should be set by the test. + Environment []StepParameter `json:"env,omitempty"` +} + +// StepParameter is a variable set by the test, with an optional default. +type StepParameter struct { + // Name of the environment variable. + Name string `json:"name"` + // Default if not set, optional, makes the parameter not required if set. + Default string `json:"default,omitempty"` + // Documentation is a textual description of the parameter. + Documentation string `json:"documentation,omitempty"` } // CredentialReference defines a secret to mount into a step and where to mount it. @@ -608,6 +620,8 @@ type MultiStageTestConfiguration struct { // Workflow is the name of the workflow to be used for this configuration. For fields defined in both // the config and the workflow, the fields from the config will override what is set in Workflow. Workflow *string `json:"workflow,omitempty"` + // Environment has the values of parameters for the steps. + Environment TestEnvironment `json:"env,omitempty"` } // MultiStageTestConfigurationLiteral is a form of the MultiStageTestConfiguration that does not include @@ -623,8 +637,13 @@ type MultiStageTestConfigurationLiteral struct { // Post is the array of test steps run after the tests finish and teardown/deprovision resources. // Post steps always run, even if previous steps fail. Post []LiteralTestStep `json:"post,omitempty"` + // Environment has the values of parameters for the steps. + Environment TestEnvironment `json:"env,omitempty"` } +// TestEnvironment has the values of parameters for multi-stage tests. +type TestEnvironment map[string]string + // Secret describes a secret to be mounted inside a test // container. type Secret struct { diff --git a/pkg/steps/multi_stage.go b/pkg/steps/multi_stage.go index 6153559edc9..622eff83bd2 100644 --- a/pkg/steps/multi_stage.go +++ b/pkg/steps/multi_stage.go @@ -50,6 +50,7 @@ type multiStageTestStep struct { config *api.ReleaseBuildConfiguration // params exposes getters for variables created by other steps params api.Parameters + env api.TestEnvironment podClient PodClient secretClient coreclientset.SecretsGetter saClient coreclientset.ServiceAccountsGetter @@ -90,21 +91,23 @@ func newMultiStageTestStep( if artifactDir != "" { artifactDir = filepath.Join(artifactDir, testConfig.As) } + ms := testConfig.MultiStageTestConfigurationLiteral return &multiStageTestStep{ logger: logger, name: testConfig.As, - profile: testConfig.MultiStageTestConfigurationLiteral.ClusterProfile, + profile: ms.ClusterProfile, config: config, params: params, + env: ms.Environment, podClient: podClient, secretClient: secretClient, saClient: saClient, rbacClient: rbacClient, artifactDir: artifactDir, jobSpec: jobSpec, - pre: testConfig.MultiStageTestConfigurationLiteral.Pre, - test: testConfig.MultiStageTestConfigurationLiteral.Test, - post: testConfig.MultiStageTestConfigurationLiteral.Post, + pre: ms.Pre, + test: ms.Test, + post: ms.Post, } } @@ -372,6 +375,7 @@ func (s *multiStageTestStep) generatePods(steps []api.LiteralTestStep, env []cor {Name: "JOB_NAME_HASH", Value: s.jobSpec.JobNameHash()}, }...) container.Env = append(container.Env, env...) + container.Env = append(container.Env, s.generateParams(step.Environment)...) if owner := s.jobSpec.Owner(); owner != nil { pod.OwnerReferences = append(pod.OwnerReferences, *owner) } @@ -413,6 +417,18 @@ func addSecretWrapper(pod *coreapi.Pod) { container.VolumeMounts = append(container.VolumeMounts, mount) } +func (s *multiStageTestStep) generateParams(env []api.StepParameter) []coreapi.EnvVar { + var ret []coreapi.EnvVar + for _, env := range env { + value := env.Default + if v, ok := s.env[env.Name]; ok { + value = v + } + ret = append(ret, coreapi.EnvVar{Name: env.Name, Value: value}) + } + return ret +} + func addSecret(secret string, pod *coreapi.Pod) { pod.Spec.Volumes = append(pod.Spec.Volumes, coreapi.Volume{ Name: secret, diff --git a/pkg/steps/multi_stage_test.go b/pkg/steps/multi_stage_test.go index 6c4c2dc8977..4dd516446ba 100644 --- a/pkg/steps/multi_stage_test.go +++ b/pkg/steps/multi_stage_test.go @@ -342,6 +342,81 @@ done } } +func TestGeneratePodsEnvironment(t *testing.T) { + value := "test" + defValue := "default" + for _, tc := range []struct { + name string + env api.TestEnvironment + test api.LiteralTestStep + expected *string + }{{ + name: "test environment is propagated to the step", + env: api.TestEnvironment{"TEST": "test"}, + test: api.LiteralTestStep{ + Environment: []api.StepParameter{{Name: "TEST"}}, + }, + expected: &value, + }, { + name: "test environment is not propagated to the step", + env: api.TestEnvironment{"TEST": "test"}, + test: api.LiteralTestStep{ + Environment: []api.StepParameter{{Name: "NOT_TEST"}}, + }, + }, { + name: "default value is overwritten", + env: api.TestEnvironment{"TEST": "test"}, + test: api.LiteralTestStep{ + Environment: []api.StepParameter{{ + Name: "TEST", + Default: "default", + }}, + }, + expected: &value, + }, { + name: "default value is applied", + test: api.LiteralTestStep{ + Environment: []api.StepParameter{{ + Name: "TEST", + Default: "default", + }}, + }, + expected: &defValue, + }} { + t.Run(tc.name, func(t *testing.T) { + jobSpec := api.JobSpec{ + Namespace: "ns", + JobSpec: prowdapi.JobSpec{ + Job: "job", + BuildID: "build_id", + ProwJobID: "prow_job_id", + Type: prowapi.PeriodicJob, + }, + } + test := []api.LiteralTestStep{tc.test} + step := MultiStageTestStep(api.TestStepConfiguration{ + MultiStageTestConfigurationLiteral: &api.MultiStageTestConfigurationLiteral{ + Test: test, + Environment: tc.env, + }, + }, &api.ReleaseBuildConfiguration{}, nil, nil, nil, nil, nil, "", &jobSpec, nil) + pods, err := step.(*multiStageTestStep).generatePods(test, nil) + if err != nil { + t.Fatal(err) + } + var env *string + for i, v := range pods[0].Spec.Containers[0].Env { + if v.Name == "TEST" { + env = &pods[0].Spec.Containers[0].Env[i].Value + } + } + if !reflect.DeepEqual(env, tc.expected) { + t.Errorf("incorrect environment:\n%s", diff.ObjectReflectDiff(env, tc.expected)) + } + }) + } +} + type fakePodExecutor struct { failures sets.String pods []*coreapi.Pod From 2b50f04d1badeae21e4c5a46be959f54ae4a23a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Barcarol=20Guimar=C3=A3es?= Date: Fri, 29 May 2020 12:22:23 +0000 Subject: [PATCH 3/6] ci-operator multi-stage: validate test parameters --- pkg/api/config.go | 37 ++++++++++++++++++++++++++++--------- pkg/api/config_test.go | 41 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/pkg/api/config.go b/pkg/api/config.go index f9d1ed81cdf..99e69304a24 100644 --- a/pkg/api/config.go +++ b/pkg/api/config.go @@ -347,9 +347,9 @@ func validateTestConfigurationType(fieldRoot string, test TestStepConfiguration, validationErrors = append(validationErrors, validateClusterProfile(fieldRoot, testConfig.ClusterProfile)...) } seen := sets.NewString() - validationErrors = append(validationErrors, validateTestSteps(fmt.Sprintf("%s.Pre", fieldRoot), testConfig.Pre, seen)...) - validationErrors = append(validationErrors, validateTestSteps(fmt.Sprintf("%s.Test", fieldRoot), testConfig.Test, seen)...) - validationErrors = append(validationErrors, validateTestSteps(fmt.Sprintf("%s.Post", fieldRoot), testConfig.Post, seen)...) + validationErrors = append(validationErrors, validateTestSteps(fmt.Sprintf("%s.Pre", fieldRoot), testConfig.Pre, seen, testConfig.Environment)...) + validationErrors = append(validationErrors, validateTestSteps(fmt.Sprintf("%s.Test", fieldRoot), testConfig.Test, seen, testConfig.Environment)...) + validationErrors = append(validationErrors, validateTestSteps(fmt.Sprintf("%s.Post", fieldRoot), testConfig.Post, seen, testConfig.Environment)...) } if testConfig := test.MultiStageTestConfigurationLiteral; testConfig != nil { typeCount++ @@ -359,15 +359,15 @@ func validateTestConfigurationType(fieldRoot string, test TestStepConfiguration, seen := sets.NewString() for i, s := range testConfig.Pre { fieldRootI := fmt.Sprintf("%s.Pre[%d]", fieldRoot, i) - validationErrors = append(validationErrors, validateLiteralTestStep(fieldRootI, s, seen)...) + validationErrors = append(validationErrors, validateLiteralTestStep(fieldRootI, s, seen, testConfig.Environment)...) } for i, s := range testConfig.Test { fieldRootI := fmt.Sprintf("%s.Test[%d]", fieldRoot, i) - validationErrors = append(validationErrors, validateLiteralTestStep(fieldRootI, s, seen)...) + validationErrors = append(validationErrors, validateLiteralTestStep(fieldRootI, s, seen, testConfig.Environment)...) } for i, s := range testConfig.Post { fieldRootI := fmt.Sprintf("%s.Post[%d]", fieldRoot, i) - validationErrors = append(validationErrors, validateLiteralTestStep(fieldRootI, s, seen)...) + validationErrors = append(validationErrors, validateLiteralTestStep(fieldRootI, s, seen, testConfig.Environment)...) } } if test.OpenshiftInstallerRandomClusterTestConfiguration != nil { @@ -386,7 +386,7 @@ func validateTestConfigurationType(fieldRoot string, test TestStepConfiguration, return validationErrors } -func validateTestSteps(fieldRoot string, steps []TestStep, seen sets.String) (ret []error) { +func validateTestSteps(fieldRoot string, steps []TestStep, seen sets.String, env TestEnvironment) (ret []error) { for i, s := range steps { fieldRootI := fmt.Sprintf("%s[%d]", fieldRoot, i) if (s.LiteralTestStep != nil && s.Reference != nil) || @@ -418,13 +418,13 @@ func validateTestSteps(fieldRoot string, steps []TestStep, seen sets.String) (re } } if s.LiteralTestStep != nil { - ret = append(ret, validateLiteralTestStep(fieldRootI, *s.LiteralTestStep, seen)...) + ret = append(ret, validateLiteralTestStep(fieldRootI, *s.LiteralTestStep, seen, env)...) } } return } -func validateLiteralTestStep(fieldRoot string, step LiteralTestStep, seen sets.String) (ret []error) { +func validateLiteralTestStep(fieldRoot string, step LiteralTestStep, seen sets.String, env TestEnvironment) (ret []error) { if len(step.As) == 0 { ret = append(ret, fmt.Errorf("%s: `as` is required", fieldRoot)) } else if seen.Has(step.As) { @@ -454,6 +454,9 @@ func validateLiteralTestStep(fieldRoot string, step LiteralTestStep, seen sets.S } ret = append(ret, validateResourceRequirements(fieldRoot+".resources", step.Resources)...) ret = append(ret, validateCredentials(fieldRoot, step.Credentials)...) + if err := validateParameters(fieldRoot, step.Environment, env); err != nil { + ret = append(ret, err) + } return } @@ -502,6 +505,22 @@ func validateCredentials(fieldRoot string, credentials []CredentialReference) [] return errs } +func validateParameters(fieldRoot string, params []StepParameter, env TestEnvironment) error { + var missing []string + for _, param := range params { + if param.Default != "" { + continue + } + if _, ok := env[param.Name]; !ok { + missing = append(missing, param.Name) + } + } + if missing != nil { + return fmt.Errorf("%s: unresolved parameter(s): %s", fieldRoot, missing) + } + return nil +} + func validateReleaseBuildConfiguration(input *ReleaseBuildConfiguration, org, repo string) []error { var validationErrors []error diff --git a/pkg/api/config_test.go b/pkg/api/config_test.go index f9f8def147b..7d9aff2d34c 100644 --- a/pkg/api/config_test.go +++ b/pkg/api/config_test.go @@ -687,7 +687,7 @@ func TestValidateTestSteps(t *testing.T) { if seen == nil { seen = sets.NewString() } - ret := validateTestSteps("test", tc.steps, seen) + ret := validateTestSteps("test", tc.steps, seen, nil) if !reflect.DeepEqual(ret, tc.errs) { t.Fatal(diff.ObjectReflectDiff(ret, tc.errs)) } @@ -695,6 +695,45 @@ func TestValidateTestSteps(t *testing.T) { } } +func TestValidateParameters(t *testing.T) { + for _, tc := range []struct { + name string + params []StepParameter + env TestEnvironment + err []error + }{{ + name: "no parameters", + }, { + name: "has parameter, parameter provided", + params: []StepParameter{{Name: "TEST"}}, + env: TestEnvironment{"TEST": "test"}, + }, { + name: "has parameter with default, no parameter provided", + params: []StepParameter{{Name: "TEST", Default: "default"}}, + }, { + name: "has parameters, some not provided", + params: []StepParameter{{Name: "TEST0"}, {Name: "TEST1"}}, + env: TestEnvironment{"TEST0": "test0"}, + err: []error{errors.New("test: unresolved parameter(s): [TEST1]")}, + }} { + t.Run(tc.name, func(t *testing.T) { + err := validateLiteralTestStep("test", LiteralTestStep{ + As: "as", + From: "from", + Commands: "commands", + Resources: ResourceRequirements{ + Requests: ResourceList{"cpu": "1"}, + Limits: ResourceList{"memory": "1m"}, + }, + Environment: tc.params, + }, sets.NewString(), tc.env) + if diff := diff.ObjectReflectDiff(err, tc.err); diff != "" { + t.Errorf("incorrect error: %s", diff) + } + }) + } +} + func TestValidateResources(t *testing.T) { for _, testCase := range []struct { name string From c0b3e02aac34d217e4ae427b745158f2be0b2056 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Barcarol=20Guimar=C3=A3es?= Date: Mon, 1 Jun 2020 15:09:55 +0000 Subject: [PATCH 4/6] registry: propagate chain environment --- pkg/api/types.go | 2 + pkg/registry/resolver.go | 84 +++++++++++++++--- pkg/registry/resolver_test.go | 163 ++++++++++++++++++++++++++++++++++ 3 files changed, 236 insertions(+), 13 deletions(-) diff --git a/pkg/api/types.go b/pkg/api/types.go index 6360daca939..85c00bdac66 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -523,6 +523,8 @@ type RegistryChain struct { Steps []TestStep `json:"steps"` // Documentation describes what the chain does. Documentation string `json:"documentation,omitempty"` + // Environment lists parameters that should be set by the test. + Environment []StepParameter `json:"env,omitempty"` } // RegistryWorkflowConfig is the struct that workflow references are unmarshalled into. diff --git a/pkg/registry/resolver.go b/pkg/registry/resolver.go index 6bfa209cd19..3ec7f886454 100644 --- a/pkg/registry/resolver.go +++ b/pkg/registry/resolver.go @@ -53,11 +53,18 @@ func (r *registry) Resolve(name string, config api.MultiStageTestConfiguration) if config.Post == nil { config.Post = workflow.Post } + if config.Environment == nil { + config.Environment = make(api.TestEnvironment, len(workflow.Environment)) + for k, v := range workflow.Environment { + config.Environment[k] = v + } + } } expandedFlow := api.MultiStageTestConfigurationLiteral{ ClusterProfile: config.ClusterProfile, } - stack := []stackRecord{stackRecord(name)} + rec := stackRecordForTest(name, config.Environment) + stack := []stackRecord{rec} pre, errs := r.process(config.Pre, sets.NewString(), stack) expandedFlow.Pre = append(expandedFlow.Pre, pre...) resolveErrors = append(resolveErrors, errs...) @@ -69,19 +76,41 @@ func (r *registry) Resolve(name string, config api.MultiStageTestConfiguration) post, errs := r.process(config.Post, sets.NewString(), stack) expandedFlow.Post = append(expandedFlow.Post, post...) resolveErrors = append(resolveErrors, errs...) - + for u := range rec.unused { + resolveErrors = append(resolveErrors, stackErrorf(stack, "no step declares parameter %q", u)) + } if resolveErrors != nil { return api.MultiStageTestConfigurationLiteral{}, errors.NewAggregate(resolveErrors) } return expandedFlow, nil } -type stackRecord string +type stackRecord struct { + name string + env []api.StepParameter + unused sets.String +} + +func stackRecordForStep(name string, env []api.StepParameter) stackRecord { + unused := sets.NewString() + for _, x := range env { + unused.Insert(x.Name) + } + return stackRecord{name: name, env: env, unused: unused} +} + +func stackRecordForTest(name string, env api.TestEnvironment) stackRecord { + params := make([]api.StepParameter, 0, len(env)) + for k, v := range env { + params = append(params, api.StepParameter{Name: k, Default: v}) + } + return stackRecordForStep(name, params) +} func stackErrorf(s []stackRecord, format string, args ...interface{}) error { var b strings.Builder for i := range s { - b.WriteString(string(s[i])) + b.WriteString(s[i].name) b.WriteString(": ") } args = append([]interface{}{b.String()}, args...) @@ -95,9 +124,9 @@ func (r *registry) process(steps []api.TestStep, seen sets.String, stack []stack errs = append(errs, err...) ret = append(ret, steps...) } else { - if step, err := r.processStep(&step, seen, stack); err != nil { - errs = append(errs, err) - } else { + step, err := r.processStep(&step, seen, stack) + errs = append(errs, err...) + if err == nil { ret = append(ret, step) } } @@ -111,26 +140,55 @@ func (r *registry) processChain(step *api.TestStep, seen sets.String, stack []st if !ok { return nil, []error{stackErrorf(stack, "unknown step chain: %s", name)} } - return r.process(chain.Steps, seen, append(stack, stackRecord(name))) + rec := stackRecordForStep(name, chain.Environment) + stack = append(stack, rec) + ret, err := r.process(chain.Steps, seen, stack) + for u := range rec.unused { + err = append(err, stackErrorf(stack, "no step declares parameter %q", u)) + } + return ret, err } -func (r *registry) processStep(step *api.TestStep, seen sets.String, stack []stackRecord) (ret api.LiteralTestStep, err error) { +func (r *registry) processStep(step *api.TestStep, seen sets.String, stack []stackRecord) (ret api.LiteralTestStep, err []error) { if ref := step.Reference; ref != nil { var ok bool ret, ok = r.stepsByName[*ref] if !ok { - return api.LiteralTestStep{}, stackErrorf(stack, "invalid step reference: %s", *ref) + return api.LiteralTestStep{}, []error{stackErrorf(stack, "invalid step reference: %s", *ref)} } } else if step.LiteralTestStep != nil { ret = *step.LiteralTestStep } else { - return api.LiteralTestStep{}, stackErrorf(stack, "encountered TestStep where both `Reference` and `LiteralTestStep` are nil") + return api.LiteralTestStep{}, []error{stackErrorf(stack, "encountered TestStep where both `Reference` and `LiteralTestStep` are nil")} } if seen.Has(ret.As) { - return api.LiteralTestStep{}, stackErrorf(stack, "duplicate name: %s", ret.As) + return api.LiteralTestStep{}, []error{stackErrorf(stack, "duplicate name: %s", ret.As)} } seen.Insert(ret.As) - return ret, nil + var errs []error + for i, e := range ret.Environment { + if v := resolveVariable(e.Name, stack); v != nil { + ret.Environment[i].Default = *v + } else if e.Default == "" { + errs = append(errs, stackErrorf(stack, "unresolved parameter: %s", e.Name)) + } + + } + return ret, errs +} + +func resolveVariable(name string, stack []stackRecord) *string { + for _, r := range stack { + for j, e := range r.env { + if e.Name == name { + for _, r := range stack { + r.unused.Delete(e.Name) + } + return &r.env[j].Default + } + } + } + return nil } // ResolveConfig uses a resolver to resolve an entire ci-operator config diff --git a/pkg/registry/resolver_test.go b/pkg/registry/resolver_test.go index c7a299d2cc1..b2aaca0ec8d 100644 --- a/pkg/registry/resolver_test.go +++ b/pkg/registry/resolver_test.go @@ -5,6 +5,7 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" "github.com/openshift/ci-tools/pkg/api" "k8s.io/apimachinery/pkg/util/diff" ) @@ -576,3 +577,165 @@ func TestResolve(t *testing.T) { }) } } + +func TestResolveParameters(t *testing.T) { + workflow := "workflow" + parent := "parent" + grandParent := "grand-parent" + grandGrandParent := "grand-grand-parent" + invalidEnv := "invalid-env" + notChanged := "not changed" + changed := "changed" + workflows := WorkflowByName{ + workflow: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{Chain: &grandGrandParent}}, + Environment: api.TestEnvironment{"CHANGED": "workflow"}, + }, + } + chains := ChainByName{ + grandGrandParent: { + Steps: []api.TestStep{{Chain: &grandParent}}, + Environment: []api.StepParameter{ + {Name: "CHANGED", Default: "grand grand parent"}, + }, + }, + grandParent: { + Steps: []api.TestStep{{Chain: &parent}}, + Environment: []api.StepParameter{ + {Name: "CHANGED", Default: "grand parent"}, + }, + }, + parent: { + Steps: []api.TestStep{ + {Reference: ¬Changed}, + {Reference: &changed}, + }, + Environment: []api.StepParameter{ + {Name: "CHANGED", Default: "parent"}, + }, + }, + invalidEnv: { + Steps: []api.TestStep{{LiteralTestStep: &api.LiteralTestStep{}}}, + Environment: []api.StepParameter{ + {Name: "NOT_DECLARED", Default: "not declared"}, + }, + }, + } + refs := ReferenceByName{ + notChanged: api.LiteralTestStep{ + As: notChanged, + Environment: []api.StepParameter{ + {Name: "NOT_CHANGED", Default: "not changed"}, + }, + }, + changed: api.LiteralTestStep{ + As: changed, + Environment: []api.StepParameter{{Name: "CHANGED"}}, + }, + } + for _, tc := range []struct { + name string + test api.MultiStageTestConfiguration + expected [][]api.StepParameter + err error + }{{ + name: "leaf, no parameters", + test: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{LiteralTestStep: &api.LiteralTestStep{}}}, + }, + expected: [][]api.StepParameter{nil}, + }, { + name: "leaf, parameters", + test: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + Environment: []api.StepParameter{ + {Name: "TEST", Default: "default"}, + }, + }, + }}, + }, + expected: [][]api.StepParameter{{{Name: "TEST", Default: "default"}}}, + }, { + name: "chain propagates to sub-steps", + test: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{Chain: &parent}}, + }, + expected: [][]api.StepParameter{ + {{Name: "NOT_CHANGED", Default: "not changed"}}, + {{Name: "CHANGED", Default: "parent"}}, + }, + }, { + name: "change propagates to sub-chains", + test: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{Chain: &grandGrandParent}}, + }, + expected: [][]api.StepParameter{ + {{Name: "NOT_CHANGED", Default: "not changed"}}, + {{Name: "CHANGED", Default: "grand grand parent"}}, + }, + }, { + name: "workflow parameter", + test: api.MultiStageTestConfiguration{Workflow: &workflow}, + expected: [][]api.StepParameter{ + {{Name: "NOT_CHANGED", Default: "not changed"}}, + {{Name: "CHANGED", Default: "workflow"}}, + }, + }, { + name: "test parameter", + test: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{Chain: &grandGrandParent}}, + Environment: api.TestEnvironment{"CHANGED": "test"}, + }, + expected: [][]api.StepParameter{ + {{Name: "NOT_CHANGED", Default: "not changed"}}, + {{Name: "CHANGED", Default: "test"}}, + }, + }, { + name: "invalid chain parameter", + test: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{Chain: &invalidEnv}}, + }, + err: errors.New(`test: invalid-env: no step declares parameter "NOT_DECLARED"`), + }, { + name: "invalid test parameter", + test: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{Reference: ¬Changed}}, + Environment: api.TestEnvironment{"NOT_DECLARED": "not declared"}, + }, + err: errors.New(`test: no step declares parameter "NOT_DECLARED"`), + }, { + name: "unresolved test", + test: api.MultiStageTestConfiguration{ + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + Environment: []api.StepParameter{{Name: "UNRESOLVED"}}, + }, + }}, + }, + err: errors.New("test: unresolved parameter: UNRESOLVED"), + }} { + t.Run(tc.name, func(t *testing.T) { + ret, err := NewResolver(refs, chains, workflows).Resolve("test", tc.test) + if tc.err != nil { + if err == nil { + t.Fatal("unexpected success") + } + if diff := cmp.Diff(err.Error(), tc.err.Error()); diff != "" { + t.Fatal(diff) + } + } else { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + var params [][]api.StepParameter + for _, s := range append(ret.Pre, append(ret.Test, ret.Post...)...) { + params = append(params, s.Environment) + } + if diff := cmp.Diff(params, tc.expected); diff != "" { + t.Error(diff) + } + } + }) + } +} From 103b03c1f936871265b5a310a7ef8f2e83bb1e70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Barcarol=20Guimar=C3=A3es?= Date: Wed, 3 Jun 2020 13:52:19 +0000 Subject: [PATCH 5/6] registry: include step name in unresolved errors --- pkg/registry/resolver.go | 2 +- pkg/registry/resolver_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/registry/resolver.go b/pkg/registry/resolver.go index 3ec7f886454..2538fdfd664 100644 --- a/pkg/registry/resolver.go +++ b/pkg/registry/resolver.go @@ -170,7 +170,7 @@ func (r *registry) processStep(step *api.TestStep, seen sets.String, stack []sta if v := resolveVariable(e.Name, stack); v != nil { ret.Environment[i].Default = *v } else if e.Default == "" { - errs = append(errs, stackErrorf(stack, "unresolved parameter: %s", e.Name)) + errs = append(errs, stackErrorf(stack, "%s: unresolved parameter: %s", ret.As, e.Name)) } } diff --git a/pkg/registry/resolver_test.go b/pkg/registry/resolver_test.go index b2aaca0ec8d..d61ea79d9b3 100644 --- a/pkg/registry/resolver_test.go +++ b/pkg/registry/resolver_test.go @@ -709,11 +709,12 @@ func TestResolveParameters(t *testing.T) { test: api.MultiStageTestConfiguration{ Test: []api.TestStep{{ LiteralTestStep: &api.LiteralTestStep{ + As: "step", Environment: []api.StepParameter{{Name: "UNRESOLVED"}}, }, }}, }, - err: errors.New("test: unresolved parameter: UNRESOLVED"), + err: errors.New("test: step: unresolved parameter: UNRESOLVED"), }} { t.Run(tc.name, func(t *testing.T) { ret, err := NewResolver(refs, chains, workflows).Resolve("test", tc.test) From 4898f8829f56e29d3e53261a18fb7925d5e50595 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Barcarol=20Guimar=C3=A3es?= Date: Thu, 4 Jun 2020 13:53:22 +0000 Subject: [PATCH 6/6] webreg: document multi-stage parameters --- pkg/webreg/webreg.go | 106 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/pkg/webreg/webreg.go b/pkg/webreg/webreg.go index 7237bf67b83..850c0ce056f 100644 --- a/pkg/webreg/webreg.go +++ b/pkg/webreg/webreg.go @@ -1033,6 +1033,11 @@ some level of encapsulation has been broken and that a more straightforward approach exists to achieve the same outcome.

+

+Parameters declared by steps and set by tests will +also be available as environment variables. +

+
Sharing Data Between Steps

Steps can communicate between each other by using a shared directory on their @@ -1188,6 +1193,65 @@ Continuing the example above, a step in the Other files that are allowed in the step registry but are not used for testing are OWNERS files and files that end in .md.

+ +

Parameters

+

+Steps, chains, and workflows can declare parameters in their env +section. These can then be set to different values to generate tests that have +small variations between them. For example: +

+ +{{ yamlSyntax (index . "paramsExample") }} + +

+A test that utilzes this step must give a value to the +OPENSHIFT_TEST_SUITE parameter, which will be available as an +environment variable when it is executed. Different tests can be generated by +setting different values, which can make generating simple variations easier. +More complex combinations are encouraged to use separate steps instead. +

+ +

+Each item in the env section consists of the following fields: +

+ +
    +
  • name: environment variable name
    • +
  • + default (optional): the value assigned if no other node in the + hierarchy provides one (described below) +
    • +
  • + documentation (optional): a textual description of the + parameter +
    • +
+ +

+ Hierarchical Propagation +

+

+Environment variables can be added to chains and workflows in the registry. +These variables will be propagated down the hierarchy. That is: a variable in +the env section of a chain will propagate to all of its sub-chains and +sub-steps, a variable in the env section of a workflow will propagate to all of +its stages. +

+ +{{ yamlSyntax (index . "paramsPropagation") }} + +

+ Required Parameters +

+

+Any variable that is not assigned a default value is considered required and +must be set at a higher level of the hierarchy. When the configuration is +resolved, tests that do not satisfy this requirement will generate a validation +failure. +

+ +{{ yamlSyntax (index . "paramsRequired") }} +{{ yamlSyntax (index . "paramsRequiredTest") }} ` const refExample = `ref: @@ -1271,6 +1335,44 @@ const configExample3 = `tests: requests: cpu: 100m memory: 200Mi` +const paramsExample = `ref: + as: openshift-e2e-test + from: tests + commands: openshift-e2e-test-commands.sh + resources: + requests: + cpu: "3" + memory: 600Mi + limits: + memory: 4Gi + env: + - name: OPENSHIFT_TEST_SUITE +` +const paramsPropagation = `chain: + as: some-chain + steps: + - ref: some-step # TEST_VARIABLE will propagate to this step + - chain: other-chain # TEST_VARIABLE will propagate to all elements in this chain + env: + - name: TEST_VARIABLE + default: test value +` +const paramsRequired = `ref: + as: some-ref + # … + env: + - name: REQUIRED_VARIABLE # automatically considered required +` +const paramsRequiredTest = `tests: +- as: valid + steps: + - ref: some-ref + env: + REQUIRED_VARIABLE: value +- as: invalid + steps: + - ref: some-ref +` const addingComponentPage = `

Adding and Changing Step Registry Content

@@ -1858,6 +1960,10 @@ func helpHandler(subPath string, w http.ResponseWriter, req *http.Request) { data["configExample1"] = configExample1 data["configExample2"] = configExample2 data["configExample3"] = configExample3 + data["paramsExample"] = paramsExample + data["paramsPropagation"] = paramsPropagation + data["paramsRequired"] = paramsRequired + data["paramsRequiredTest"] = paramsRequiredTest case "/adding-components": helpTemplate, err = helpFuncs.Parse(addingComponentPage) case "/examples":