From 29d34452b17716c0a251700458fe515476203008 Mon Sep 17 00:00:00 2001 From: James Rawlings Date: Sat, 28 Nov 2020 10:41:19 +0000 Subject: [PATCH] refactor: move save and load functions from requirements config to top level requirements object --- pkg/apis/core/v4beta1/register.go | 1 + pkg/apis/core/v4beta1/requirements.go | 44 +++++++------ pkg/apis/core/v4beta1/requirements_test.go | 63 +++++++++++-------- .../core/v4beta1/zz_generated.deepcopy.go | 9 ++- 4 files changed, 71 insertions(+), 46 deletions(-) diff --git a/pkg/apis/core/v4beta1/register.go b/pkg/apis/core/v4beta1/register.go index 054c8a8..7b587a5 100644 --- a/pkg/apis/core/v4beta1/register.go +++ b/pkg/apis/core/v4beta1/register.go @@ -45,6 +45,7 @@ func addKnownTypes(scheme *runtime.Scheme) error { &Plugin{}, &PipelineActivity{}, &PipelineActivityList{}, + &Requirements{}, &Release{}, &ReleaseList{}, &SourceRepository{}, diff --git a/pkg/apis/core/v4beta1/requirements.go b/pkg/apis/core/v4beta1/requirements.go index 9395b38..f811f97 100644 --- a/pkg/apis/core/v4beta1/requirements.go +++ b/pkg/apis/core/v4beta1/requirements.go @@ -146,11 +146,10 @@ var RepositoryTypeValues = []string{string(RepositoryTypeNone), string(Repositor // Requirements represents a collection installation requirements for Jenkins X // +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +k8s:openapi-gen=true type Requirements struct { metav1.TypeMeta `json:",inline"` - // +optional - metav1.ObjectMeta `json:"metadata"` // Spec the definition of the secret mappings Spec RequirementsConfig `json:"spec"` @@ -437,17 +436,23 @@ type RequirementsConfig struct { } // NewRequirementsConfig creates a default configuration file -func NewRequirementsConfig() *RequirementsConfig { - return &RequirementsConfig{ - SecretStorage: SecretStorageTypeLocal, - Webhook: WebhookTypeLighthouse, +func NewRequirementsConfig() *Requirements { + return &Requirements{ + TypeMeta: metav1.TypeMeta{ + Kind: "Requirements", // todo not sure if this is correct but not able to find how TypeMeta gets set when type is plain file rather than incluster CRD, perhaps it's automated when in cluster? + APIVersion: SchemeGroupVersion.String(), + }, + Spec: RequirementsConfig{ + SecretStorage: SecretStorageTypeLocal, + Webhook: WebhookTypeLighthouse, + }, } } // LoadRequirementsConfig loads the project configuration if there is a project configuration file // if there is not a file called `jx-requirements.yml` in the given dir we will scan up the parent // directories looking for the requirements file as we often run 'jx' steps in sub directories. -func LoadRequirementsConfig(dir string, failOnValidationErrors bool) (*RequirementsConfig, string, error) { +func LoadRequirementsConfig(dir string, failOnValidationErrors bool) (*Requirements, string, error) { absolute, err := filepath.Abs(dir) if err != nil { return nil, "", errors.Wrap(err, "creating absolute path") @@ -465,16 +470,16 @@ func LoadRequirementsConfig(dir string, failOnValidationErrors bool) (*Requireme continue } - config, err := LoadRequirementsConfigFile(fileName, failOnValidationErrors) - return config, fileName, err + requirements, err := LoadRequirementsConfigFile(fileName, failOnValidationErrors) + return requirements, fileName, err } return nil, "", errors.New("jx-requirements.yml file not found") } // LoadRequirementsConfigFile loads a specific project YAML configuration file -func LoadRequirementsConfigFile(fileName string, failOnValidationErrors bool) (*RequirementsConfig, error) { +func LoadRequirementsConfigFile(fileName string, failOnValidationErrors bool) (*Requirements, error) { - config := &RequirementsConfig{} + requirements := NewRequirementsConfig() _, err := os.Stat(fileName) if err != nil { return nil, errors.Wrapf(err, "checking if file %s exists", fileName) @@ -488,7 +493,7 @@ func LoadRequirementsConfigFile(fileName string, failOnValidationErrors bool) (* s := string(data) // //check whether new or old jx requirements if strings.Contains(s, "apiVersion") && strings.Contains(s, "kind") && strings.Contains(s, "spec") { - requirements := &Requirements{} + validationErrors, err := util.ValidateYaml(requirements, data) if err != nil { return nil, fmt.Errorf("failed to validate YAML file %s due to %s", fileName, err) @@ -506,8 +511,8 @@ func LoadRequirementsConfigFile(fileName string, failOnValidationErrors bool) (* return nil, fmt.Errorf("failed to unmarshal YAML file %s due to %s", fileName, err) } - config = &requirements.Spec } else { + config := &RequirementsConfig{} validationErrors, err := util.ValidateYaml(config, data) if err != nil { return nil, fmt.Errorf("failed to validate YAML file %s due to %s", fileName, err) @@ -525,10 +530,12 @@ func LoadRequirementsConfigFile(fileName string, failOnValidationErrors bool) (* if err != nil { return nil, fmt.Errorf("failed to unmarshal YAML file %s due to %s", fileName, err) } + + requirements.Spec = *config } - config.addDefaults() - return config, nil + requirements.Spec.addDefaults() + return requirements, nil } // GetRequirementsConfigFromTeamSettings reads the BootRequirements string from TeamSettings and unmarshals it @@ -564,8 +571,7 @@ func (c *RequirementsConfig) IsEmpty() bool { return reflect.DeepEqual(empty, c) } -// SaveConfig saves the configuration file to the given project directory -func (c *RequirementsConfig) SaveConfig(fileName string) error { +func (c *Requirements) SaveConfig(fileName string) error { data, err := yaml.Marshal(c) if err != nil { return err @@ -628,12 +634,12 @@ func (t sliceTransformer) Transformer(typ reflect.Type) func(dst, src reflect.Va return nil } -// MergeSave attempts to merge the provided RequirementsConfig with the caller's data. +// MergeSave attempts to merge the provided Requirements with the caller's data. // It does so overriding values in the source struct with non-zero values from the provided struct // it defines non-zero per property and not for a while embedded struct, meaning that nested properties // in embedded structs should also be merged correctly. // if a slice is added a transformer will be needed to handle correctly merging the contained values -func (c *RequirementsConfig) MergeSave(src *RequirementsConfig, requirementsFileName string) error { +func (c *Requirements) MergeSave(src *Requirements, requirementsFileName string) error { err := mergo.Merge(c, src, mergo.WithOverride, mergo.WithTransformers(sliceTransformer{})) if err != nil { return errors.Wrap(err, "error merging jx-requirements.yml files") diff --git a/pkg/apis/core/v4beta1/requirements_test.go b/pkg/apis/core/v4beta1/requirements_test.go index 2204543..e616a61 100644 --- a/pkg/apis/core/v4beta1/requirements_test.go +++ b/pkg/apis/core/v4beta1/requirements_test.go @@ -39,7 +39,8 @@ func TestRequirementsConfigMarshalExistingFile(t *testing.T) { expectedPipelineUserEmail := "someone@acme.com" file := filepath.Join(dir, v4beta1.RequirementsConfigFileName) - requirements := v4beta1.NewRequirementsConfig() + requirementResource := v4beta1.NewRequirementsConfig() + requirements := &requirementResource.Spec requirements.SecretStorage = expectedSecretStorage requirements.Cluster.ClusterName = expectedClusterName requirements.Ingress.Domain = expectedDomain @@ -48,23 +49,23 @@ func TestRequirementsConfigMarshalExistingFile(t *testing.T) { Email: expectedPipelineUserEmail, } - err = requirements.SaveConfig(file) + err = requirementResource.SaveConfig(file) assert.NoError(t, err, "failed to save file %s", file) - requirements, fileName, err := v4beta1.LoadRequirementsConfig(dir, v4beta1.DefaultFailOnValidationError) + requirementResource, fileName, err := v4beta1.LoadRequirementsConfig(dir, v4beta1.DefaultFailOnValidationError) assert.NoError(t, err, "failed to load requirements file in dir %s", dir) assert.FileExists(t, fileName) - + requirements = &requirementResource.Spec assert.Equal(t, expectedClusterName, requirements.Cluster.ClusterName, "requirements.ClusterName") assert.Equal(t, expectedSecretStorage, requirements.SecretStorage, "requirements.SecretStorage") assert.Equal(t, expectedDomain, requirements.Ingress.Domain, "requirements.Domain") // lets check we can load the file from a sub dir subDir := filepath.Join(dir, "subdir") - requirements, fileName, err = v4beta1.LoadRequirementsConfig(subDir, v4beta1.DefaultFailOnValidationError) + requirementResource, fileName, err = v4beta1.LoadRequirementsConfig(subDir, v4beta1.DefaultFailOnValidationError) assert.NoError(t, err, "failed to load requirements file in subDir: %s", subDir) assert.FileExists(t, fileName) - + requirements = &requirementResource.Spec t.Logf("generated requirements file %s\n", fileName) assert.Equal(t, expectedClusterName, requirements.Cluster.ClusterName, "requirements.ClusterName") @@ -78,10 +79,10 @@ func TestRequirementsConfigMarshalExistingFile(t *testing.T) { } func Test_OverrideRequirementsFromEnvironment_does_not_initialise_nil_structs(t *testing.T) { - requirements, fileName, err := v4beta1.LoadRequirementsConfig(testDataDir, v4beta1.DefaultFailOnValidationError) + requirementResource, fileName, err := v4beta1.LoadRequirementsConfig(testDataDir, v4beta1.DefaultFailOnValidationError) assert.NoError(t, err, "failed to load requirements file in dir %s", testDataDir) assert.FileExists(t, fileName) - + requirements := &requirementResource.Spec requirements.OverrideRequirementsFromEnvironment(func(in string) (string, error) { return "", nil }) @@ -92,7 +93,7 @@ func Test_OverrideRequirementsFromEnvironment_does_not_initialise_nil_structs(t _ = os.RemoveAll(tempDir) }() - err = requirements.SaveConfig(filepath.Join(tempDir, v4beta1.RequirementsConfigFileName)) + err = requirementResource.SaveConfig(filepath.Join(tempDir, v4beta1.RequirementsConfigFileName)) assert.NoError(t, err, "failed to save requirements file in dir %s", tempDir) _, fileName, err = v4beta1.LoadRequirementsConfig(tempDir, v4beta1.DefaultFailOnValidationError) @@ -211,9 +212,8 @@ func TestRequirementsConfigMarshalExistingFileKanikoFalse(t *testing.T) { assert.NoError(t, err, "should create a temporary config dir") file := filepath.Join(dir, v4beta1.RequirementsConfigFileName) - requirements := v4beta1.NewRequirementsConfig() - - err = requirements.SaveConfig(file) + requirementsResource := v4beta1.NewRequirementsConfig() + err = requirementsResource.SaveConfig(file) assert.NoError(t, err, "failed to save file %s", file) _, fileName, err := v4beta1.LoadRequirementsConfig(dir, v4beta1.DefaultFailOnValidationError) @@ -237,8 +237,8 @@ func TestRequirementsConfigMarshalInEmptyDir(t *testing.T) { func TestRequirementsConfigIngressAutoDNS(t *testing.T) { t.Parallel() - requirements := v4beta1.NewRequirementsConfig() - + requirementsResource := v4beta1.NewRequirementsConfig() + requirements := &requirementsResource.Spec requirements.Ingress.Domain = "1.2.3.4.nip.io" assert.Equal(t, true, requirements.Ingress.IsAutoDNSDomain(), "requirements.Ingress.IsAutoDNSDomain() for domain %s", requirements.Ingress.Domain) @@ -276,7 +276,8 @@ func Test_marshalling_empty_requirements_config_creates_no_build_pack_configurat func Test_marshalling_vault_config(t *testing.T) { t.Parallel() - requirements := v4beta1.NewRequirementsConfig() + requirementsResource := v4beta1.NewRequirementsConfig() + requirements := &requirementsResource.Spec requirements.Vault = v4beta1.VaultConfig{ Name: "myVault", URL: "http://myvault", @@ -314,12 +315,12 @@ func Test_env_repository_visibility(t *testing.T) { content, err := ioutil.ReadFile(path.Join(testDataDir, testCase.yamlFile)) assert.NoError(t, err) - requirementsConfig := v4beta1.NewRequirementsConfig() - + requirementsResource := v4beta1.NewRequirementsConfig() + requirements := &requirementsResource.Spec _ = log.CaptureOutput(func() { - err = yaml.Unmarshal(content, requirementsConfig) + err = yaml.Unmarshal(content, requirements) assert.NoError(t, err) - assert.Equal(t, testCase.expectedGitPublic, requirementsConfig.Cluster.EnvironmentGitPublic, "unexpected value for repository visibility") + assert.Equal(t, testCase.expectedGitPublic, requirements.Cluster.EnvironmentGitPublic, "unexpected value for repository visibility") }) }) } @@ -473,9 +474,15 @@ func TestMergeSave(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { - err = tc.Original.MergeSave(tc.Changed, f.Name()) + r := v4beta1.Requirements{ + Spec: *tc.Original, + } + rChanged := &v4beta1.Requirements{ + Spec: *tc.Changed, + } + err = r.MergeSave(rChanged, f.Name()) assert.NoError(t, err, "the merge shouldn't fail for case %s", tc.Name) - tc.ValidationFunc(tc.Original, tc.Changed) + tc.ValidationFunc(&r.Spec, &rChanged.Spec) }) } } @@ -484,8 +491,9 @@ func Test_EnvironmentGitPublic_and_EnvironmentGitPrivate_specified_together_retu content, err := ioutil.ReadFile(path.Join(testDataDir, "git_public_true_git_private_true.yaml")) assert.NoError(t, err) - requirementsConfig := v4beta1.NewRequirementsConfig() - err = yaml.Unmarshal(content, requirementsConfig) + requirementsResource := v4beta1.NewRequirementsConfig() + requirements := &requirementsResource.Spec + err = yaml.Unmarshal(content, requirements) assert.Error(t, err) assert.Contains(t, err.Error(), "only EnvironmentGitPublic should be used") } @@ -525,15 +533,17 @@ func Test_LoadRequirementsConfig(t *testing.T) { require.NoError(t, err, "unable to write requirements file %s", expectedRequirementsFile) } - requirements, requirementsFile, err := v4beta1.LoadRequirementsConfig(testPath, v4beta1.DefaultFailOnValidationError) + requirementsResource, requirementsFile, err := v4beta1.LoadRequirementsConfig(testPath, v4beta1.DefaultFailOnValidationError) + if testCase.createRequirements { + requirements := &requirementsResource.Spec require.NoError(t, err) assert.Equal(t, expectedRequirementsFile, requirementsFile) assert.Equal(t, string(requirements.Webhook), "prow") } else { require.Error(t, err) assert.Equal(t, "", requirementsFile) - assert.Nil(t, requirements) + assert.Nil(t, requirementsResource) } }) } @@ -561,7 +571,8 @@ func TestBackwardsCompatibleRequirementsFile(t *testing.T) { } func validateRequirements(t *testing.T, oldRequirementsDir string) { - requirements, fileName, err := v4beta1.LoadRequirementsConfig(oldRequirementsDir, true) + requirementsResource, fileName, err := v4beta1.LoadRequirementsConfig(oldRequirementsDir, true) + requirements := &requirementsResource.Spec assert.NoError(t, err, "failed to load old style jx-requirements.yml") assert.NotEmpty(t, fileName, "requirements filename should not be empty") assert.NotNil(t, requirements, "requirements should not be empty") diff --git a/pkg/apis/core/v4beta1/zz_generated.deepcopy.go b/pkg/apis/core/v4beta1/zz_generated.deepcopy.go index d5f7a21..e685df7 100644 --- a/pkg/apis/core/v4beta1/zz_generated.deepcopy.go +++ b/pkg/apis/core/v4beta1/zz_generated.deepcopy.go @@ -1119,7 +1119,6 @@ func (in *ReleaseStatus) DeepCopy() *ReleaseStatus { func (in *Requirements) DeepCopyInto(out *Requirements) { *out = *in out.TypeMeta = in.TypeMeta - in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) return } @@ -1134,6 +1133,14 @@ func (in *Requirements) DeepCopy() *Requirements { return out } +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *Requirements) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RequirementsConfig) DeepCopyInto(out *RequirementsConfig) { *out = *in