Merge pull request #87 from rawlingsj/master

refactor: move save and load functions from requirements config to to…

pkg/apis/core/v4beta1/register.go

@@ -45,6 +45,7 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 		&Plugin{},
 		&PipelineActivity{},
 		&PipelineActivityList{},
+		&Requirements{},
 		&Release{},
 		&ReleaseList{},
 		&SourceRepository{},

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")

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")