From ca71de3951b1750804de19615e66a05a5f8af5e5 Mon Sep 17 00:00:00 2001 From: Ruslan Aliev Date: Mon, 16 Nov 2020 19:24:06 -0600 Subject: [PATCH] Move document filesystem to a separate package In order to widely use filesystem interface in airshipctl there is need to move it to separate package to avoid importing unnecessary dependencies from document package and, as a consequence, possible cyclic dependencies. * filesystem moved from document/fs to pkg/fs Change-Id: I3b6298462f03db43594a9fa26bf23ab7687c5589 Signed-off-by: Ruslan Aliev Relates-To: #415 --- pkg/clusterctl/client/executor_test.go | 15 ++++++----- pkg/document/bundle.go | 15 ++++++----- pkg/{document => fs}/filesystem.go | 12 ++++----- pkg/k8s/applier/executor_test.go | 9 ++++--- pkg/k8s/kubeconfig/builder.go | 10 +++---- pkg/k8s/kubeconfig/kubeconfig.go | 21 ++++++++------- pkg/k8s/kubeconfig/kubeconfig_test.go | 36 +++++++++++++------------- pkg/k8s/kubectl/kubectl.go | 7 ++--- pkg/k8s/kubectl/kubectl_test.go | 21 ++++++++------- testutil/fs/fs.go | 14 +++++----- testutil/testdatafs.go | 7 ++--- 11 files changed, 87 insertions(+), 80 deletions(-) rename pkg/{document => fs}/filesystem.go (90%) diff --git a/pkg/clusterctl/client/executor_test.go b/pkg/clusterctl/client/executor_test.go index 54dd6616a..a73725570 100644 --- a/pkg/clusterctl/client/executor_test.go +++ b/pkg/clusterctl/client/executor_test.go @@ -33,10 +33,11 @@ import ( "opendev.org/airship/airshipctl/pkg/document" airerrors "opendev.org/airship/airshipctl/pkg/errors" "opendev.org/airship/airshipctl/pkg/events" + "opendev.org/airship/airshipctl/pkg/fs" "opendev.org/airship/airshipctl/pkg/k8s/kubeconfig" "opendev.org/airship/airshipctl/pkg/phase" "opendev.org/airship/airshipctl/pkg/phase/ifc" - "opendev.org/airship/airshipctl/testutil/fs" + testfs "opendev.org/airship/airshipctl/testutil/fs" ) var ( @@ -103,7 +104,7 @@ func TestExecutorRun(t *testing.T) { testCases := []struct { name string cfgDoc document.Document - fs document.FileSystem + fs fs.FileSystem bundlePath string expectedEvt []events.Event clusterMap clustermap.ClusterMap @@ -120,8 +121,8 @@ func TestExecutorRun(t *testing.T) { { name: "Error temporary file", cfgDoc: executorDoc(t, "init"), - fs: fs.MockFileSystem{ - MockTempFile: func(string, string) (document.File, error) { + fs: testfs.MockFileSystem{ + MockTempFile: func(string, string) (fs.File, error) { return nil, errTmpFile }, }, @@ -137,9 +138,9 @@ func TestExecutorRun(t *testing.T) { { name: "Regular Run init", cfgDoc: executorDoc(t, "init"), - fs: fs.MockFileSystem{ - MockTempFile: func(string, string) (document.File, error) { - return fs.TestFile{ + fs: testfs.MockFileSystem{ + MockTempFile: func(string, string) (fs.File, error) { + return testfs.TestFile{ MockName: func() string { return "filename" }, MockWrite: func() (int, error) { return 0, nil }, MockClose: func() error { return nil }, diff --git a/pkg/document/bundle.go b/pkg/document/bundle.go index 274312042..34671203a 100644 --- a/pkg/document/bundle.go +++ b/pkg/document/bundle.go @@ -24,6 +24,7 @@ import ( "sigs.k8s.io/kustomize/api/resource" "sigs.k8s.io/kustomize/api/types" + "opendev.org/airship/airshipctl/pkg/fs" utilyaml "opendev.org/airship/airshipctl/pkg/util/yaml" ) @@ -37,14 +38,14 @@ type KustomizeBuildOptions struct { type BundleFactory struct { KustomizeBuildOptions resmap.ResMap - FileSystem + fs.FileSystem } // Bundle interface provides the specification for a bundle implementation type Bundle interface { Write(out io.Writer) error - SetFileSystem(FileSystem) error - GetFileSystem() FileSystem + SetFileSystem(fs.FileSystem) error + GetFileSystem() fs.FileSystem Select(selector Selector) ([]Document, error) SelectOne(selector Selector) (Document, error) SelectBundle(selector Selector) (Bundle, error) @@ -64,13 +65,13 @@ type BundleFactoryFunc func() (Bundle, error) // NewBundleByPath is a function which builds new document.Bundle from kustomize rootPath using default FS object // example: document.NewBundleByPath("path/to/phase-root") func NewBundleByPath(rootPath string) (Bundle, error) { - return NewBundle(NewDocumentFs(), rootPath) + return NewBundle(fs.NewDocumentFs(), rootPath) } // NewBundle is a convenience function to create a new bundle // Over time, it will evolve to support allowing more control // for kustomize plugins -func NewBundle(fSys FileSystem, kustomizePath string) (Bundle, error) { +func NewBundle(fSys fs.FileSystem, kustomizePath string) (Bundle, error) { var options = KustomizeBuildOptions{ KustomizationPath: kustomizePath, LoadRestrictions: types.LoadRestrictionsRootOnly, @@ -132,13 +133,13 @@ func (b *BundleFactory) SetKustomizeBuildOptions(k KustomizeBuildOptions) error } // SetFileSystem sets the filesystem that will be used by this bundle -func (b *BundleFactory) SetFileSystem(fSys FileSystem) error { +func (b *BundleFactory) SetFileSystem(fSys fs.FileSystem) error { b.FileSystem = fSys return nil } // GetFileSystem gets the filesystem that will be used by this bundle -func (b *BundleFactory) GetFileSystem() FileSystem { +func (b *BundleFactory) GetFileSystem() fs.FileSystem { return b.FileSystem } diff --git a/pkg/document/filesystem.go b/pkg/fs/filesystem.go similarity index 90% rename from pkg/document/filesystem.go rename to pkg/fs/filesystem.go index eb51bd6a9..c7bc28597 100644 --- a/pkg/document/filesystem.go +++ b/pkg/fs/filesystem.go @@ -12,35 +12,35 @@ limitations under the License. */ -package document +package fs import ( "io/ioutil" - fs "sigs.k8s.io/kustomize/api/filesys" + kustfs "sigs.k8s.io/kustomize/api/filesys" ) // File extends kustomize File and provide abstraction to creating temporary files type File interface { - fs.File + kustfs.File Name() string } // FileSystem extends kustomize FileSystem and provide abstraction to creating temporary files type FileSystem interface { - fs.FileSystem + kustfs.FileSystem TempFile(string, string) (File, error) TempDir(string, string) (string, error) } // Fs is adaptor to TempFile type Fs struct { - fs.FileSystem + kustfs.FileSystem } // NewDocumentFs returns an instance of Fs func NewDocumentFs() FileSystem { - return &Fs{FileSystem: fs.MakeFsOnDisk()} + return &Fs{FileSystem: kustfs.MakeFsOnDisk()} } // TempFile creates file in temporary filesystem, at default os.TempDir diff --git a/pkg/k8s/applier/executor_test.go b/pkg/k8s/applier/executor_test.go index c21b227ca..1a712df08 100644 --- a/pkg/k8s/applier/executor_test.go +++ b/pkg/k8s/applier/executor_test.go @@ -26,12 +26,13 @@ import ( "opendev.org/airship/airshipctl/pkg/config" "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/events" + "opendev.org/airship/airshipctl/pkg/fs" "opendev.org/airship/airshipctl/pkg/k8s/applier" "opendev.org/airship/airshipctl/pkg/k8s/kubeconfig" "opendev.org/airship/airshipctl/pkg/k8s/utils" "opendev.org/airship/airshipctl/pkg/phase" "opendev.org/airship/airshipctl/pkg/phase/ifc" - "opendev.org/airship/airshipctl/testutil/fs" + testfs "opendev.org/airship/airshipctl/testutil/fs" ) const ( @@ -279,9 +280,9 @@ func testKubeconfig(stringData string) kubeconfig.Interface { return kubeconfig.NewKubeConfig( kubeconfig.FromByte([]byte(stringData)), kubeconfig.InjectFileSystem( - fs.MockFileSystem{ - MockTempFile: func(root, pattern string) (document.File, error) { - return fs.TestFile{ + testfs.MockFileSystem{ + MockTempFile: func(root, pattern string) (fs.File, error) { + return testfs.TestFile{ MockName: func() string { return "kubeconfig-142398" }, MockWrite: func() (int, error) { return 0, nil }, MockClose: func() error { return nil }, diff --git a/pkg/k8s/kubeconfig/builder.go b/pkg/k8s/kubeconfig/builder.go index 4ebcec5e0..da05c594f 100644 --- a/pkg/k8s/kubeconfig/builder.go +++ b/pkg/k8s/kubeconfig/builder.go @@ -19,8 +19,8 @@ import ( "opendev.org/airship/airshipctl/pkg/cluster/clustermap" "opendev.org/airship/airshipctl/pkg/config" - "opendev.org/airship/airshipctl/pkg/document" "opendev.org/airship/airshipctl/pkg/errors" + "opendev.org/airship/airshipctl/pkg/fs" "opendev.org/airship/airshipctl/pkg/util" ) @@ -77,8 +77,8 @@ func (b *Builder) WithTempRoot(root string) *Builder { func (b *Builder) Build() Interface { switch { case b.path != "": - fs := document.NewDocumentFs() - return NewKubeConfig(FromFile(b.path, fs), InjectFilePath(b.path, fs), InjectTempRoot(b.root)) + fSys := fs.NewDocumentFs() + return NewKubeConfig(FromFile(b.path, fSys), InjectFilePath(b.path, fSys), InjectTempRoot(b.root)) case b.fromParent(): // TODO add method that would get kubeconfig from parent cluster and glue it together // with parent kubeconfig if needed @@ -88,10 +88,10 @@ func (b *Builder) Build() Interface { case b.bundlePath != "": return NewKubeConfig(FromBundle(b.bundlePath), InjectTempRoot(b.root)) default: - fs := document.NewDocumentFs() + fSys := fs.NewDocumentFs() // return default path to kubeconfig file in airship workdir path := filepath.Join(util.UserHomeDir(), config.AirshipConfigDir, KubeconfigDefaultFileName) - return NewKubeConfig(FromFile(path, fs), InjectFilePath(path, fs), InjectTempRoot(b.root)) + return NewKubeConfig(FromFile(path, fSys), InjectFilePath(path, fSys), InjectTempRoot(b.root)) } } diff --git a/pkg/k8s/kubeconfig/kubeconfig.go b/pkg/k8s/kubeconfig/kubeconfig.go index 0caa04fba..a63fa869d 100644 --- a/pkg/k8s/kubeconfig/kubeconfig.go +++ b/pkg/k8s/kubeconfig/kubeconfig.go @@ -22,6 +22,7 @@ import ( "opendev.org/airship/airshipctl/pkg/api/v1alpha1" "opendev.org/airship/airshipctl/pkg/document" + "opendev.org/airship/airshipctl/pkg/fs" ) // Interface provides a uniform way to interact with kubeconfig file @@ -44,7 +45,7 @@ type kubeConfig struct { path string dumpRoot string - fileSystem document.FileSystem + fileSystem fs.FileSystem sourceFunc KubeSourceFunc } @@ -60,7 +61,7 @@ func NewKubeConfig(source KubeSourceFunc, options ...Option) Interface { } kf.sourceFunc = source if kf.fileSystem == nil { - kf.fileSystem = document.NewDocumentFs() + kf.fileSystem = fs.NewDocumentFs() } return kf } @@ -96,9 +97,9 @@ func FromSecret(kubeOpts *FromClusterOptions) KubeSourceFunc { } // FromFile returns KubeSource type, uses path to kubeconfig on FS as source to construct kubeconfig object -func FromFile(path string, fs document.FileSystem) KubeSourceFunc { +func FromFile(path string, fSys fs.FileSystem) KubeSourceFunc { return func() ([]byte, error) { - return fs.ReadFile(path) + return fSys.ReadFile(path) } } @@ -130,9 +131,9 @@ func FromBundle(root string) KubeSourceFunc { } // InjectFileSystem sets fileSystem to be used, mostly to be used for tests -func InjectFileSystem(fs document.FileSystem) Option { +func InjectFileSystem(fSys fs.FileSystem) Option { return func(k *kubeConfig) { - k.fileSystem = fs + k.fileSystem = fSys } } @@ -146,10 +147,10 @@ func InjectTempRoot(dumpRoot string) Option { // InjectFilePath enables setting kubeconfig path, useful when you have kubeconfig // from the actual filesystem, if this option is used, please also make sure that // FromFile option is also used as a first argument in NewKubeConfig function -func InjectFilePath(path string, fs document.FileSystem) Option { +func InjectFilePath(path string, fSys fs.FileSystem) Option { return func(k *kubeConfig) { k.path = path - k.fileSystem = fs + k.fileSystem = fSys } } @@ -202,12 +203,12 @@ func (k *kubeConfig) GetFile() (string, Cleanup, error) { return k.WriteTempFile(k.dumpRoot) } -func cleanup(path string, fs document.FileSystem) Cleanup { +func cleanup(path string, fSys fs.FileSystem) Cleanup { if path == "" { return func() {} } return func() { - if err := fs.RemoveAll(path); err != nil { + if err := fSys.RemoveAll(path); err != nil { log.Fatalf("Failed to cleanup kubeconfig file %s, error: %v", path, err) } } diff --git a/pkg/k8s/kubeconfig/kubeconfig_test.go b/pkg/k8s/kubeconfig/kubeconfig_test.go index 28cab6071..089066ae7 100644 --- a/pkg/k8s/kubeconfig/kubeconfig_test.go +++ b/pkg/k8s/kubeconfig/kubeconfig_test.go @@ -29,10 +29,10 @@ import ( kustfs "sigs.k8s.io/kustomize/api/filesys" "opendev.org/airship/airshipctl/pkg/api/v1alpha1" - "opendev.org/airship/airshipctl/pkg/document" + "opendev.org/airship/airshipctl/pkg/fs" "opendev.org/airship/airshipctl/pkg/k8s/client/fake" "opendev.org/airship/airshipctl/pkg/k8s/kubeconfig" - "opendev.org/airship/airshipctl/testutil/fs" + testfs "opendev.org/airship/airshipctl/testutil/fs" ) const ( @@ -123,15 +123,15 @@ var ( func TestKubeconfigContent(t *testing.T) { expectedData := []byte(testValidKubeconfig) - fs := document.NewDocumentFs() + fSys := fs.NewDocumentFs() kubeconf := kubeconfig.NewKubeConfig( kubeconfig.FromByte(expectedData), - kubeconfig.InjectFileSystem(fs), + kubeconfig.InjectFileSystem(fSys), kubeconfig.InjectTempRoot(".")) path, clean, err := kubeconf.GetFile() require.NoError(t, err) defer clean() - actualData, err := fs.ReadFile(path) + actualData, err := fSys.ReadFile(path) require.NoError(t, err) assert.Equal(t, expectedData, actualData) } @@ -357,9 +357,9 @@ func TestNewKubeConfig(t *testing.T) { src: kubeconfig.FromByte([]byte(testValidKubeconfig)), options: []kubeconfig.Option{ kubeconfig.InjectFileSystem( - fs.MockFileSystem{ - MockTempFile: func(root, pattern string) (document.File, error) { - return fs.TestFile{ + testfs.MockFileSystem{ + MockTempFile: func(root, pattern string) (fs.File, error) { + return testfs.TestFile{ MockName: func() string { return "kubeconfig-142398" }, MockWrite: func() (int, error) { return 0, nil }, MockClose: func() error { return nil }, @@ -378,13 +378,13 @@ func TestNewKubeConfig(t *testing.T) { options: []kubeconfig.Option{ kubeconfig.InjectTempRoot("/my-unique-root"), kubeconfig.InjectFileSystem( - fs.MockFileSystem{ - MockTempFile: func(root, _ string) (document.File, error) { + testfs.MockFileSystem{ + MockTempFile: func(root, _ string) (fs.File, error) { // check if root path is passed to the TempFile interface if root != "/my-unique-root" { return nil, errTempFile } - return fs.TestFile{ + return testfs.TestFile{ MockName: func() string { return "kubeconfig-142398" }, MockWrite: func() (int, error) { return 0, nil }, MockClose: func() error { return nil }, @@ -419,8 +419,8 @@ func TestNewKubeConfig(t *testing.T) { expectedErrorContains: errTempFile.Error(), options: []kubeconfig.Option{ kubeconfig.InjectFileSystem( - fs.MockFileSystem{ - MockTempFile: func(string, string) (document.File, error) { + testfs.MockFileSystem{ + MockTempFile: func(string, string) (fs.File, error) { return nil, errTempFile }, MockRemoveAll: func() error { return nil }, @@ -505,7 +505,7 @@ func TestKubeConfigWriteFile(t *testing.T) { path string expectedErrorContains string - fs document.FileSystem + fs fs.FileSystem src kubeconfig.KubeSourceFunc }{ { @@ -537,8 +537,8 @@ func TestKubeConfigWriteFile(t *testing.T) { } } -func readFile(t *testing.T, path string, fs document.FileSystem) string { - b, err := fs.ReadFile(path) +func readFile(t *testing.T, path string, fSys fs.FileSystem) string { + b, err := fSys.ReadFile(path) require.NoError(t, err) return string(b) } @@ -549,8 +549,8 @@ func read(t *testing.T, r io.Reader) string { return string(b) } -func fsWithFile(t *testing.T, path string) document.FileSystem { - fSys := fs.MockFileSystem{ +func fsWithFile(t *testing.T, path string) fs.FileSystem { + fSys := testfs.MockFileSystem{ FileSystem: kustfs.MakeFsInMemory(), MockRemoveAll: func() error { return nil diff --git a/pkg/k8s/kubectl/kubectl.go b/pkg/k8s/kubectl/kubectl.go index 545ce1358..7311a7199 100644 --- a/pkg/k8s/kubectl/kubectl.go +++ b/pkg/k8s/kubectl/kubectl.go @@ -22,6 +22,7 @@ import ( cmdutil "k8s.io/kubectl/pkg/cmd/util" "opendev.org/airship/airshipctl/pkg/document" + "opendev.org/airship/airshipctl/pkg/fs" "opendev.org/airship/airshipctl/pkg/log" utilyaml "opendev.org/airship/airshipctl/pkg/util/yaml" ) @@ -31,7 +32,7 @@ import ( type Kubectl struct { cmdutil.Factory genericclioptions.IOStreams - document.FileSystem + fs.FileSystem // Directory to buffer documents before passing them to kubectl commands // default is empty, this means that /tmp dir will be used bufferDir string @@ -47,7 +48,7 @@ func NewKubectl(f cmdutil.Factory) *Kubectl { Out: os.Stdout, ErrOut: os.Stderr, }, - FileSystem: document.NewDocumentFs(), + FileSystem: fs.NewDocumentFs(), } } @@ -77,7 +78,7 @@ func (kubectl *Kubectl) ApplyYaml(yaml []byte, ao *ApplyOptions) error { return err } - defer func(f document.File) { + defer func(f fs.File) { fName := f.Name() dErr := kubectl.RemoveAll(fName) if dErr != nil { diff --git a/pkg/k8s/kubectl/kubectl_test.go b/pkg/k8s/kubectl/kubectl_test.go index cce363474..dcee73c1d 100644 --- a/pkg/k8s/kubectl/kubectl_test.go +++ b/pkg/k8s/kubectl/kubectl_test.go @@ -23,10 +23,11 @@ import ( corev1 "k8s.io/api/core/v1" "opendev.org/airship/airshipctl/pkg/document" + "opendev.org/airship/airshipctl/pkg/fs" "opendev.org/airship/airshipctl/pkg/k8s/kubectl" k8sutils "opendev.org/airship/airshipctl/pkg/k8s/utils" "opendev.org/airship/airshipctl/testutil" - "opendev.org/airship/airshipctl/testutil/fs" + testfs "opendev.org/airship/airshipctl/testutil/fs" k8stest "opendev.org/airship/airshipctl/testutil/k8sutils" ) @@ -73,14 +74,14 @@ func TestApply(t *testing.T) { tests := []struct { name string expectedErr error - fs document.FileSystem + fs fs.FileSystem }{ { expectedErr: nil, - fs: fs.MockFileSystem{ + fs: testfs.MockFileSystem{ MockRemoveAll: func() error { return nil }, - MockTempFile: func(string, string) (document.File, error) { - return fs.TestFile{ + MockTempFile: func(string, string) (fs.File, error) { + return testfs.TestFile{ MockName: func() string { return filenameRC }, MockWrite: func() (int, error) { return 0, nil }, MockClose: func() error { return nil }, @@ -90,15 +91,15 @@ func TestApply(t *testing.T) { }, { expectedErr: ErrWriteOutError, - fs: fs.MockFileSystem{ - MockTempFile: func(string, string) (document.File, error) { return nil, ErrWriteOutError }}, + fs: testfs.MockFileSystem{ + MockTempFile: func(string, string) (fs.File, error) { return nil, ErrWriteOutError }}, }, { expectedErr: ErrTempFileError, - fs: fs.MockFileSystem{ + fs: testfs.MockFileSystem{ MockRemoveAll: func() error { return nil }, - MockTempFile: func(string, string) (document.File, error) { - return fs.TestFile{ + MockTempFile: func(string, string) (fs.File, error) { + return testfs.TestFile{ MockWrite: func() (int, error) { return 0, ErrTempFileError }, MockName: func() string { return filenameRC }, MockClose: func() error { return nil }, diff --git a/testutil/fs/fs.go b/testutil/fs/fs.go index d34d878cc..9a6f69052 100644 --- a/testutil/fs/fs.go +++ b/testutil/fs/fs.go @@ -15,27 +15,27 @@ package fs import ( - fs "sigs.k8s.io/kustomize/api/filesys" + kustfs "sigs.k8s.io/kustomize/api/filesys" - "opendev.org/airship/airshipctl/pkg/document" + "opendev.org/airship/airshipctl/pkg/fs" ) -var _ document.FileSystem = MockFileSystem{} +var _ fs.FileSystem = MockFileSystem{} // MockFileSystem implements Filesystem type MockFileSystem struct { MockRemoveAll func() error MockTempDir func() (string, error) // allow to check content of the incoming parameters, root and patter for temp file - MockTempFile func(string, string) (document.File, error) - fs.FileSystem + MockTempFile func(string, string) (fs.File, error) + kustfs.FileSystem } // RemoveAll Filesystem interface implementation func (fsys MockFileSystem) RemoveAll(string) error { return fsys.MockRemoveAll() } // TempFile Filesystem interface implementation -func (fsys MockFileSystem) TempFile(root, pattern string) (document.File, error) { +func (fsys MockFileSystem) TempFile(root, pattern string) (fs.File, error) { return fsys.MockTempFile(root, pattern) } @@ -46,7 +46,7 @@ func (fsys MockFileSystem) TempDir(string, string) (string, error) { // TestFile implements file type TestFile struct { - document.File + fs.File MockName func() string MockWrite func() (int, error) MockClose func() error diff --git a/testutil/testdatafs.go b/testutil/testdatafs.go index 3c08875da..8a3fbfa22 100644 --- a/testutil/testdatafs.go +++ b/testutil/testdatafs.go @@ -22,19 +22,20 @@ import ( fixtures "github.com/go-git/go-git-fixtures/v4" "github.com/stretchr/testify/require" - fs "sigs.k8s.io/kustomize/api/filesys" + kustfs "sigs.k8s.io/kustomize/api/filesys" "opendev.org/airship/airshipctl/pkg/document" + "opendev.org/airship/airshipctl/pkg/fs" ) // SetupTestFs help manufacture a fake file system for testing purposes. It // will iterate over the files in fixtureDir, which is a directory relative // to the tests themselves, and will write each of those files (preserving // names) to an in-memory file system and return that fs -func SetupTestFs(t *testing.T, fixtureDir string) document.FileSystem { +func SetupTestFs(t *testing.T, fixtureDir string) fs.FileSystem { t.Helper() - x := &document.Fs{FileSystem: fs.MakeFsInMemory()} + x := &fs.Fs{FileSystem: kustfs.MakeFsInMemory()} files, err := ioutil.ReadDir(fixtureDir) require.NoErrorf(t, err, "Failed to read fixture directory %s", fixtureDir)