From f9fdf4eaa9c135116d68b2138f826a4f50acb18c Mon Sep 17 00:00:00 2001 From: Ruslan Aliev Date: Fri, 9 Oct 2020 08:30:50 -0500 Subject: [PATCH] Use document filesystem in config object to read/write files It allows us to have more clear and controllable unit tests and to move towards bringing all file operations in airshipctl to a common standard for all source files (using only document filesystem module). Change-Id: I560edb7c8e9cd682f3c188527245fcd40045e16c Signed-off-by: Ruslan Aliev Relates-To: #415 --- pkg/config/config.go | 38 ++++++++++++++++---------- pkg/config/config_test.go | 21 ++++++-------- pkg/config/utils.go | 3 ++ pkg/phase/helper_test.go | 6 ++-- pkg/util/configreader.go | 31 --------------------- pkg/util/configreader_test.go | 47 -------------------------------- pkg/util/testdata/incorrect.yaml | 3 -- pkg/util/testdata/test.yaml | 1 - testutil/testconfig.go | 39 +++++++++++++------------- 9 files changed, 57 insertions(+), 132 deletions(-) delete mode 100644 pkg/util/configreader.go delete mode 100644 pkg/util/configreader_test.go delete mode 100644 pkg/util/testdata/incorrect.yaml delete mode 100644 pkg/util/testdata/test.yaml diff --git a/pkg/config/config.go b/pkg/config/config.go index 0cdf3fa47..0f998eb10 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -16,13 +16,13 @@ package config import ( "fmt" - "io/ioutil" "os" "path/filepath" "sort" "sigs.k8s.io/yaml" + "opendev.org/airship/airshipctl/pkg/fs" "opendev.org/airship/airshipctl/pkg/log" "opendev.org/airship/airshipctl/pkg/util" ) @@ -62,6 +62,7 @@ type Config struct { // file from which this config was loaded // +not persisted in file loadedConfigPath string + fileSystem fs.FileSystem } // Permissions has the permissions for file and directory @@ -125,11 +126,12 @@ func (c *Config) initConfigPath(airshipConfigPath string) { func (c *Config) LoadConfig() error { // If I can read from the file, load from it // throw an error otherwise - if _, err := os.Stat(c.loadedConfigPath); err != nil { + data, err := c.fileSystem.ReadFile(c.loadedConfigPath) + if err != nil { return err } - return util.ReadYAMLFile(c.loadedConfigPath, c) + return yaml.Unmarshal(data, c) } // EnsureComplete verifies that a Config object is ready to use. @@ -189,26 +191,26 @@ func (c *Config) PersistConfig(overwrite bool) error { } // WriteFile doesn't create the directory, create it if needed - configDir := filepath.Dir(c.loadedConfigPath) - err = os.MkdirAll(configDir, os.FileMode(c.Permissions.DirectoryPermission)) - if err != nil { - return err - } - - // Write the Airship Config file - err = ioutil.WriteFile(c.loadedConfigPath, airshipConfigYaml, os.FileMode(c.Permissions.FilePermission)) + dir := c.fileSystem.Dir(c.loadedConfigPath) + err = c.fileSystem.MkdirAll(dir) if err != nil { return err } // Change the permission of directory - err = os.Chmod(configDir, os.FileMode(c.Permissions.DirectoryPermission)) + err = c.fileSystem.Chmod(dir, os.FileMode(c.Permissions.DirectoryPermission)) + if err != nil { + return err + } + + // Write the Airship Config file + err = c.fileSystem.WriteFile(c.loadedConfigPath, airshipConfigYaml) if err != nil { return err } // Change the permission of config file - err = os.Chmod(c.loadedConfigPath, os.FileMode(c.Permissions.FilePermission)) + err = c.fileSystem.Chmod(c.loadedConfigPath, os.FileMode(c.Permissions.FilePermission)) if err != nil { return err } @@ -551,7 +553,7 @@ func (c *Config) CurrentContextManagementConfig() (*ManagementConfiguration, err // Purge removes the config file func (c *Config) Purge() error { - return os.Remove(c.loadedConfigPath) + return c.fileSystem.RemoveAll(c.loadedConfigPath) } // CurrentContextManifestMetadata gets manifest metadata @@ -569,7 +571,13 @@ func (c *Config) CurrentContextManifestMetadata() (*Metadata, error) { Inventory: &InventoryMeta{}, PhaseMeta: &PhaseMeta{}, } - err = util.ReadYAMLFile(filepath.Join(manifest.TargetPath, phaseRepoDir, manifest.MetadataPath), meta) + + data, err := c.fileSystem.ReadFile(filepath.Join(manifest.TargetPath, phaseRepoDir, manifest.MetadataPath)) + if err != nil { + return nil, err + } + + err = yaml.Unmarshal(data, meta) if err != nil { return nil, err } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 105eb7c4a..21018d457 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -16,7 +16,9 @@ package config_test import ( "fmt" + "io/ioutil" "os" + "path/filepath" "strings" "testing" @@ -96,19 +98,14 @@ func TestLoadConfig(t *testing.T) { } func TestPersistConfig(t *testing.T) { - conf, cleanup := testutil.InitConfig(t) - defer cleanup(t) - - conf.SetLoadedConfigPath(conf.LoadedConfigPath() + ".new") - - err := conf.PersistConfig(true) + testDir, err := ioutil.TempDir("", "airship-test") + require.NoError(t, err) + configPath := filepath.Join(testDir, "config") + err = config.CreateConfig(configPath, true) + require.NoError(t, err) + assert.FileExists(t, configPath) + err = os.RemoveAll(testDir) require.NoError(t, err) - - // Check that the files were created - assert.FileExists(t, conf.LoadedConfigPath()) - - err = conf.PersistConfig(false) - require.Error(t, err, config.ErrConfigFileExists{Path: conf.LoadedConfigPath()}) } func TestEnsureComplete(t *testing.T) { diff --git a/pkg/config/utils.go b/pkg/config/utils.go index 584a5164d..b2e135db9 100644 --- a/pkg/config/utils.go +++ b/pkg/config/utils.go @@ -16,6 +16,8 @@ package config import ( "encoding/base64" + + "opendev.org/airship/airshipctl/pkg/fs" ) // NewConfig returns a newly initialized Config object @@ -53,6 +55,7 @@ func NewConfig() *Config { MetadataPath: DefaultManifestMetadataFile, }, }, + fileSystem: fs.NewDocumentFs(), } } diff --git a/pkg/phase/helper_test.go b/pkg/phase/helper_test.go index 438767970..b95b67503 100644 --- a/pkg/phase/helper_test.go +++ b/pkg/phase/helper_test.go @@ -555,9 +555,9 @@ manifests: metadataPath: valid_site/metadata.yaml repositories: primary: - url: "empty/filename/" -` - conf := &config.Config{} + url: "empty/filename/"` + + conf := config.NewConfig() err := yaml.Unmarshal([]byte(confString), conf) require.NoError(t, err) return conf diff --git a/pkg/util/configreader.go b/pkg/util/configreader.go deleted file mode 100644 index f82109337..000000000 --- a/pkg/util/configreader.go +++ /dev/null @@ -1,31 +0,0 @@ -/* - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - https://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package util - -import ( - "io/ioutil" - - "sigs.k8s.io/yaml" -) - -// ReadYAMLFile reads YAML-formatted configuration file and -// de-serializes it to a given object -func ReadYAMLFile(filePath string, cfg interface{}) error { - data, err := ioutil.ReadFile(filePath) - if err != nil { - return err - } - return yaml.Unmarshal(data, cfg) -} diff --git a/pkg/util/configreader_test.go b/pkg/util/configreader_test.go deleted file mode 100644 index 565ba50ec..000000000 --- a/pkg/util/configreader_test.go +++ /dev/null @@ -1,47 +0,0 @@ -/* - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - https://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package util_test - -import ( - "testing" - - "github.com/stretchr/testify/assert" - - "opendev.org/airship/airshipctl/pkg/util" -) - -func TestReadYAMLFile(t *testing.T) { - assert := assert.New(t) - - var actual map[string]interface{} - - tests := []struct { - testFile string - isError bool - }{ - {"testdata/test.yaml", false}, - {"testdata/incorrect.yaml", true}, - {"testdata/notfound.yaml", true}, - } - - for _, test := range tests { - err := util.ReadYAMLFile(test.testFile, &actual) - if test.isError { - assert.Error(err) - } else { - assert.NoError(err) - } - } -} diff --git a/pkg/util/testdata/incorrect.yaml b/pkg/util/testdata/incorrect.yaml deleted file mode 100644 index 23840f08c..000000000 --- a/pkg/util/testdata/incorrect.yaml +++ /dev/null @@ -1,3 +0,0 @@ --- -testString: incorrectYamlSyntax -... diff --git a/pkg/util/testdata/test.yaml b/pkg/util/testdata/test.yaml deleted file mode 100644 index cb8585cdc..000000000 --- a/pkg/util/testdata/test.yaml +++ /dev/null @@ -1 +0,0 @@ -testString: test diff --git a/testutil/testconfig.go b/testutil/testconfig.go index b9c14e38d..63c7f81d4 100644 --- a/testutil/testconfig.go +++ b/testutil/testconfig.go @@ -29,27 +29,26 @@ import ( // DummyConfig used by tests, to initialize min set of data func DummyConfig() *config.Config { - conf := &config.Config{ - Kind: config.AirshipConfigKind, - APIVersion: config.AirshipConfigAPIVersion, - Permissions: config.Permissions{ - DirectoryPermission: config.AirshipDefaultDirectoryPermission, - FilePermission: config.AirshipDefaultFilePermission, - }, - Contexts: map[string]*config.Context{ - "dummy_context": DummyContext(), - }, - Manifests: map[string]*config.Manifest{ - "dummy_manifest": DummyManifest(), - }, - ManagementConfiguration: map[string]*config.ManagementConfiguration{ - "dummy_management_config": DummyManagementConfiguration(), - }, - EncryptionConfigs: map[string]*config.EncryptionConfig{ - "dummy_encryption_config": DummyEncryptionConfig(), - }, - CurrentContext: "dummy_context", + conf := config.NewConfig() + conf.Kind = config.AirshipConfigKind + conf.APIVersion = config.AirshipConfigAPIVersion + conf.Permissions = config.Permissions{ + DirectoryPermission: config.AirshipDefaultDirectoryPermission, + FilePermission: config.AirshipDefaultFilePermission, } + conf.Contexts = map[string]*config.Context{ + "dummy_context": DummyContext(), + } + conf.Manifests = map[string]*config.Manifest{ + "dummy_manifest": DummyManifest(), + } + conf.ManagementConfiguration = map[string]*config.ManagementConfiguration{ + "dummy_management_config": DummyManagementConfiguration(), + } + conf.EncryptionConfigs = map[string]*config.EncryptionConfig{ + "dummy_encryption_config": DummyEncryptionConfig(), + } + conf.CurrentContext = "dummy_context" return conf }