diff --git a/pkg/model/file/interfaces.go b/pkg/model/file/interfaces.go new file mode 100644 index 00000000000..28b6af52e3e --- /dev/null +++ b/pkg/model/file/interfaces.go @@ -0,0 +1,84 @@ +/* +Copyright 2018 The Kubernetes Authors. + +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 + + http://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 file + +import ( + "sigs.k8s.io/kubebuilder/pkg/model/resource" +) + +// Builder defines the basic methods that any file builder must implement +type Builder interface { + // GetPath returns the path to the file location + GetPath() string + // GetIfExistsAction returns the behavior when creating a file that already exists + GetIfExistsAction() IfExistsAction +} + +// RequiresValidation is a file builder that requires validation +type RequiresValidation interface { + Builder + // Validate returns true if the template has valid values + Validate() error +} + +// Template is file builder based on a file template +type Template interface { + Builder + // GetBody returns the template body + GetBody() string + // SetTemplateDefaults returns the TemplateMixin for creating a scaffold file + SetTemplateDefaults() error +} + +// Inserter is a file builder that inserts code fragments in marked positions +type Inserter interface { + Builder + // GetMarkers returns the different markers where code fragments will be inserted + GetMarkers() []Marker + // GetCodeFragments returns a map that binds markers to code fragments + GetCodeFragments() CodeFragmentsMap +} + +// HasDomain allows the domain to be used on a template +type HasDomain interface { + // InjectDomain sets the template domain + InjectDomain(string) +} + +// HasRepository allows the repository to be used on a template +type HasRepository interface { + // InjectRepository sets the template repository + InjectRepository(string) +} + +// HasMultiGroup allows the multi-group flag to be used on a template +type HasMultiGroup interface { + // InjectMultiGroup sets the template multi-group flag + InjectMultiGroup(bool) +} + +// HasBoilerplate allows a boilerplate to be used on a template +type HasBoilerplate interface { + // InjectBoilerplate sets the template boilerplate + InjectBoilerplate(string) +} + +// HasResource allows a resource to be used on a template +type HasResource interface { + // InjectResource sets the template resource + InjectResource(*resource.Resource) +} diff --git a/pkg/model/file/marker.go b/pkg/model/file/marker.go new file mode 100644 index 00000000000..a2e2de1a038 --- /dev/null +++ b/pkg/model/file/marker.go @@ -0,0 +1,56 @@ +/* +Copyright 2019 The Kubernetes Authors. + +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 + + http://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 file + +import ( + "fmt" + "path/filepath" +) + +const prefix = "+kubebuilder:scaffold:" + +var commentsByExt = map[string]string{ + // TODO(v3): machine-readable comments should not have spaces by Go convention. However, + // this is a backwards incompatible change, and thus should be done for next project version. + ".go": "// ", + ".yaml": "# ", +} + +// Marker represents a comment LoC that will be used to insert code fragments by update operations +type Marker struct { + comment string + value string +} + +// NewMarkerFor creates a new marker customized for the specific file +func NewMarkerFor(path string, value string) Marker { + ext := filepath.Ext(path) + if comment, found := commentsByExt[ext]; found { + return Marker{comment, value} + } + + panic(fmt.Errorf("unknown file extension: '%s', expected '.go' or '.yaml'", ext)) +} + +// String implements Stringer +func (m Marker) String() string { + return m.comment + prefix + m.value +} + +type CodeFragments []string + +type CodeFragmentsMap map[Marker]CodeFragments diff --git a/pkg/model/file/template.go b/pkg/model/file/mixins.go similarity index 60% rename from pkg/model/file/template.go rename to pkg/model/file/mixins.go index 7702ed5d18d..64bf886c59d 100644 --- a/pkg/model/file/template.go +++ b/pkg/model/file/mixins.go @@ -20,46 +20,50 @@ import ( "sigs.k8s.io/kubebuilder/pkg/model/resource" ) -// Template is a scaffoldable file template -type Template interface { - // GetPath returns the path to the file location - GetPath() string - // GetBody returns the template body - GetBody() string - // GetIfExistsAction returns the behavior when creating a file that already exists - GetIfExistsAction() IfExistsAction - // SetTemplateDefaults returns the TemplateMixin for creating a scaffold file - SetTemplateDefaults() error -} - -// TemplateMixin is the input for scaffolding a file -type TemplateMixin struct { - // Path is the file to write +// PathMixin provides file builders with a path field +type PathMixin struct { + // Path is the of the file Path string +} - // TemplateBody is the template body to execute - TemplateBody string +// GetPath implements Builder +func (t *PathMixin) GetPath() string { + return t.Path +} +// IfExistsActionMixin provides file builders with a if-exists-action field +type IfExistsActionMixin struct { // IfExistsAction determines what to do if the file exists IfExistsAction IfExistsAction } -func (t *TemplateMixin) GetPath() string { - return t.Path +// GetIfExistsAction implements Builder +func (t *IfExistsActionMixin) GetIfExistsAction() IfExistsAction { + return t.IfExistsAction +} + +// TemplateMixin is the mixin that should be embedded in Template builders +type TemplateMixin struct { + PathMixin + IfExistsActionMixin + + // TemplateBody is the template body to execute + TemplateBody string } func (t *TemplateMixin) GetBody() string { return t.TemplateBody } -func (t *TemplateMixin) GetIfExistsAction() IfExistsAction { - return t.IfExistsAction +// InserterMixin is the mixin that should be embedded in Inserter builders +type InserterMixin struct { + PathMixin } -// HasDomain allows the domain to be used on a template -type HasDomain interface { - // InjectDomain sets the template domain - InjectDomain(string) +// GetIfExistsAction implements Builder +func (t *InserterMixin) GetIfExistsAction() IfExistsAction { + // Inserter builders always need to overwrite previous files + return Overwrite } // DomainMixin provides templates with a injectable domain field @@ -75,12 +79,6 @@ func (m *DomainMixin) InjectDomain(domain string) { } } -// HasRepository allows the repository to be used on a template -type HasRepository interface { - // InjectRepository sets the template repository - InjectRepository(string) -} - // RepositoryMixin provides templates with a injectable repository field type RepositoryMixin struct { // Repo is the go project package path @@ -94,12 +92,6 @@ func (m *RepositoryMixin) InjectRepository(repository string) { } } -// HasMultiGroup allows the multi-group flag to be used on a template -type HasMultiGroup interface { - // InjectMultiGroup sets the template multi-group flag - InjectMultiGroup(bool) -} - // MultiGroupMixin provides templates with a injectable multi-group flag field type MultiGroupMixin struct { // MultiGroup is the multi-group flag @@ -111,12 +103,6 @@ func (m *MultiGroupMixin) InjectMultiGroup(flag bool) { m.MultiGroup = flag } -// HasBoilerplate allows a boilerplate to be used on a template -type HasBoilerplate interface { - // InjectBoilerplate sets the template boilerplate - InjectBoilerplate(string) -} - // BoilerplateMixin provides templates with a injectable boilerplate field type BoilerplateMixin struct { // Boilerplate is the contents of a Boilerplate go header file @@ -130,12 +116,6 @@ func (m *BoilerplateMixin) InjectBoilerplate(boilerplate string) { } } -// HasResource allows a resource to be used on a template -type HasResource interface { - // InjectResource sets the template resource - InjectResource(*resource.Resource) -} - // ResourceMixin provides templates with a injectable resource field type ResourceMixin struct { Resource *resource.Resource @@ -147,10 +127,3 @@ func (m *ResourceMixin) InjectResource(res *resource.Resource) { m.Resource = res } } - -// RequiresValidation is a file that requires validation -type RequiresValidation interface { - Template - // Validate returns true if the template has valid values - Validate() error -} diff --git a/pkg/model/universe.go b/pkg/model/universe.go index 73cffa734ea..cf2878e5a95 100644 --- a/pkg/model/universe.go +++ b/pkg/model/universe.go @@ -34,7 +34,7 @@ type Universe struct { Resource *resource.Resource `json:"resource,omitempty"` // Files contains the model of the files that are being scaffolded - Files []*file.File `json:"files,omitempty"` + Files map[string]*file.File `json:"files,omitempty"` } // NewUniverse creates a new Universe @@ -78,7 +78,7 @@ func WithResource(resource *resource.Resource) UniverseOption { } } -func (u Universe) InjectInto(t file.Template) { +func (u Universe) InjectInto(t file.Builder) { // Inject project configuration if u.Config != nil { if templateWithDomain, hasDomain := t.(file.HasDomain); hasDomain { diff --git a/pkg/scaffold/api.go b/pkg/scaffold/api.go index ea89a8b7a41..eed382c9bc5 100644 --- a/pkg/scaffold/api.go +++ b/pkg/scaffold/api.go @@ -162,19 +162,14 @@ func (s *apiScaffolder) scaffoldV2() error { return fmt.Errorf("error scaffolding APIs: %v", err) } - kustomizationFile := &crdv2.Kustomization{} if err := machinery.NewScaffold().Execute( s.newUniverse(), - kustomizationFile, + &crdv2.Kustomization{}, &crdv2.KustomizeConfig{}, ); err != nil { return fmt.Errorf("error scaffolding kustomization: %v", err) } - if err := kustomizationFile.Update(); err != nil { - return fmt.Errorf("error updating kustomization.yaml: %v", err) - } - } else { // disable generation of example reconcile body if not scaffolding resource // because this could result in a fork-bomb of k8s resources where watching a @@ -192,27 +187,18 @@ func (s *apiScaffolder) scaffoldV2() error { fmt.Sprintf("%s_controller.go", strings.ToLower(s.resource.Kind)))) } - suiteTestFile := &controllerv2.SuiteTest{} if err := machinery.NewScaffold(s.plugins...).Execute( s.newUniverse(), - suiteTestFile, + &controllerv2.SuiteTest{}, &controllerv2.Controller{}, ); err != nil { return fmt.Errorf("error scaffolding controller: %v", err) } - - if err := suiteTestFile.Update(); err != nil { - return fmt.Errorf("error updating suite_test.go under controllers pkg: %v", err) - } } - if err := (&templatesv2.Main{}).Update( - &templatesv2.MainUpdateOptions{ - Config: &s.config.Config, - WireResource: s.doResource, - WireController: s.doController, - Resource: s.resource, - }, + if err := machinery.NewScaffold(s.plugins...).Execute( + s.newUniverse(), + &templatesv2.MainUpdater{WireResource: s.doResource, WireController: s.doController}, ); err != nil { return fmt.Errorf("error updating main.go: %v", err) } diff --git a/pkg/scaffold/init.go b/pkg/scaffold/init.go index dbc2749e34f..8e1800f34b0 100644 --- a/pkg/scaffold/init.go +++ b/pkg/scaffold/init.go @@ -23,7 +23,6 @@ import ( "sigs.k8s.io/kubebuilder/internal/config" "sigs.k8s.io/kubebuilder/pkg/model" - "sigs.k8s.io/kubebuilder/pkg/model/file" "sigs.k8s.io/kubebuilder/pkg/scaffold/internal/machinery" templatesv1 "sigs.k8s.io/kubebuilder/pkg/scaffold/internal/templates/v1" managerv1 "sigs.k8s.io/kubebuilder/pkg/scaffold/internal/templates/v1/manager" @@ -86,13 +85,13 @@ func (s *initScaffolder) Scaffold() error { } func (s *initScaffolder) scaffoldV1() error { + bpFile := &templatesv1.Boilerplate{} + bpFile.Path = s.boilerplatePath + bpFile.License = s.license + bpFile.Owner = s.owner if err := machinery.NewScaffold().Execute( s.newUniverse(""), - &templatesv1.Boilerplate{ - TemplateMixin: file.TemplateMixin{Path: s.boilerplatePath}, - License: s.license, - Owner: s.owner, - }, + bpFile, ); err != nil { return err } @@ -126,13 +125,13 @@ func (s *initScaffolder) scaffoldV1() error { } func (s *initScaffolder) scaffoldV2() error { + bpFile := &templatesv2.Boilerplate{} + bpFile.Path = s.boilerplatePath + bpFile.License = s.license + bpFile.Owner = s.owner if err := machinery.NewScaffold().Execute( s.newUniverse(""), - &templatesv2.Boilerplate{ - TemplateMixin: file.TemplateMixin{Path: s.boilerplatePath}, - License: s.license, - Owner: s.owner, - }, + bpFile, ); err != nil { return err } diff --git a/pkg/scaffold/internal/filesystem/errors.go b/pkg/scaffold/internal/filesystem/errors.go index 6591eb074dc..c09a1e01d2e 100644 --- a/pkg/scaffold/internal/filesystem/errors.go +++ b/pkg/scaffold/internal/filesystem/errors.go @@ -24,6 +24,38 @@ import ( // They are not exported as they should not be created outside of this package // Exported functions are provided to check which kind of error was returned +// fileExistsError is returned if it could not be checked if the file exists +type fileExistsError struct { + path string + err error +} + +func (e fileExistsError) Error() string { + return fmt.Sprintf("failed to check if %s exists: %v", e.path, e.err) +} + +// IsFileExistsError checks if the returned error is because the file could not be checked for existence +func IsFileExistsError(e error) bool { + _, ok := e.(fileExistsError) + return ok +} + +// openFileError is returned if the file could not be opened +type openFileError struct { + path string + err error +} + +func (e openFileError) Error() string { + return fmt.Sprintf("failed to open %s: %v", e.path, e.err) +} + +// IsOpenFileError checks if the returned error is because the file could not be opened +func IsOpenFileError(e error) bool { + _, ok := e.(openFileError) + return ok +} + // createDirectoryError is returned if the directory could not be created type createDirectoryError struct { path string @@ -34,8 +66,7 @@ func (e createDirectoryError) Error() string { return fmt.Sprintf("failed to create directory for %s: %v", e.path, e.err) } -// IsCreateDirectoryError checks if the returned error is because the directory -// could not be created +// IsCreateDirectoryError checks if the returned error is because the directory could not be created func IsCreateDirectoryError(e error) bool { _, ok := e.(createDirectoryError) return ok @@ -51,14 +82,29 @@ func (e createFileError) Error() string { return fmt.Sprintf("failed to create %s: %v", e.path, e.err) } -// IsCreateFileError checks if the returned error is because the file could not -// be created +// IsCreateFileError checks if the returned error is because the file could not be created func IsCreateFileError(e error) bool { _, ok := e.(createFileError) return ok } -// writeFileError is returned if the filed could not be written to +// readFileError is returned if the file could not be read +type readFileError struct { + path string + err error +} + +func (e readFileError) Error() string { + return fmt.Sprintf("failed to read from %s: %v", e.path, e.err) +} + +// IsReadFileError checks if the returned error is because the file could not be read +func IsReadFileError(e error) bool { + _, ok := e.(readFileError) + return ok +} + +// writeFileError is returned if the file could not be written type writeFileError struct { path string err error @@ -68,8 +114,7 @@ func (e writeFileError) Error() string { return fmt.Sprintf("failed to write to %s: %v", e.path, e.err) } -// IsWriteFileError checks if the returned error is because the file could not -// be written to +// IsWriteFileError checks if the returned error is because the file could not be written func IsWriteFileError(e error) bool { _, ok := e.(writeFileError) return ok @@ -85,8 +130,7 @@ func (e closeFileError) Error() string { return fmt.Sprintf("failed to close %s: %v", e.path, e.err) } -// IsCloseFileError checks if the returned error is because the file could not -// be closed +// IsCloseFileError checks if the returned error is because the file could not be closed func IsCloseFileError(e error) bool { _, ok := e.(closeFileError) return ok diff --git a/pkg/scaffold/internal/filesystem/filesystem.go b/pkg/scaffold/internal/filesystem/filesystem.go index fd10583e415..04a32883c3f 100644 --- a/pkg/scaffold/internal/filesystem/filesystem.go +++ b/pkg/scaffold/internal/filesystem/filesystem.go @@ -36,6 +36,9 @@ type FileSystem interface { // Exists checks if the file exists Exists(path string) (bool, error) + // Open opens the file and returns a self-closing io.Reader. + Open(path string) (io.ReadCloser, error) + // Create creates the directory and file and returns a self-closing // io.Writer pointing to that file. If the file exists, it truncates it. Create(path string) (io.Writer, error) @@ -87,7 +90,22 @@ func FilePermissions(filePerm os.FileMode) Options { // Exists implements FileSystem.Exists func (fs fileSystem) Exists(path string) (bool, error) { - return afero.Exists(fs.fs, path) + exists, err := afero.Exists(fs.fs, path) + if err != nil { + return false, fileExistsError{path, err} + } + + return exists, nil +} + +// Open implements FileSystem.Open +func (fs fileSystem) Open(path string) (io.ReadCloser, error) { + rc, err := fs.fs.Open(path) + if err != nil { + return nil, openFileError{path, err} + } + + return &readFile{path, rc}, nil } // Create implements FileSystem.Create @@ -103,17 +121,49 @@ func (fs fileSystem) Create(path string) (io.Writer, error) { return nil, createFileError{path, err} } - return &file{path, wc}, nil + return &writeFile{path, wc}, nil +} + +var _ io.ReadCloser = &readFile{} + +// readFile implements io.Reader +type readFile struct { + path string + io.ReadCloser +} + +// Read implements io.Reader.ReadCloser +func (f *readFile) Read(content []byte) (n int, err error) { + // Read the content + n, err = f.ReadCloser.Read(content) + // EOF is a special case error that we can't wrap + if err == io.EOF { + return + } + if err != nil { + return n, readFileError{f.path, err} + } + + return n, nil +} + +// Close implements io.Reader.ReadCloser +func (f *readFile) Close() error { + if err := f.ReadCloser.Close(); err != nil { + return closeFileError{f.path, err} + } + + return nil } -// file implements io.Writer -type file struct { +// writeFile implements io.Writer +type writeFile struct { path string io.WriteCloser } // Write implements io.Writer.Write -func (f *file) Write(content []byte) (n int, err error) { +func (f *writeFile) Write(content []byte) (n int, err error) { // Close the file when we end writing defer func() { if closeErr := f.Close(); err == nil && closeErr != nil { diff --git a/pkg/scaffold/internal/filesystem/filesystem_test.go b/pkg/scaffold/internal/filesystem/filesystem_test.go index 60bc1c0f88a..3cebc6da6d2 100644 --- a/pkg/scaffold/internal/filesystem/filesystem_test.go +++ b/pkg/scaffold/internal/filesystem/filesystem_test.go @@ -157,12 +157,47 @@ var _ = Describe("FileSystem", func() { var ( err = errors.New("test error") path = filepath.Join("path", "to", "file") + fileExistsErr = fileExistsError{path, err} + openFileErr = openFileError{path, err} createDirectoryErr = createDirectoryError{path, err} createFileErr = createFileError{path, err} + readFileErr = readFileError{path, err} writeFileErr = writeFileError{path, err} closeFileErr = closeFileError{path, err} ) + Context("IsFileExistsError", func() { + It("should return true for file exists errors", func() { + Expect(IsFileExistsError(fileExistsErr)).To(BeTrue()) + }) + + It("should return false for any other error", func() { + Expect(IsFileExistsError(err)).To(BeFalse()) + Expect(IsFileExistsError(openFileErr)).To(BeFalse()) + Expect(IsFileExistsError(createDirectoryErr)).To(BeFalse()) + Expect(IsFileExistsError(createFileErr)).To(BeFalse()) + Expect(IsFileExistsError(readFileErr)).To(BeFalse()) + Expect(IsFileExistsError(writeFileErr)).To(BeFalse()) + Expect(IsFileExistsError(closeFileErr)).To(BeFalse()) + }) + }) + + Context("IsOpenFileError", func() { + It("should return true for open file errors", func() { + Expect(IsOpenFileError(openFileErr)).To(BeTrue()) + }) + + It("should return false for any other error", func() { + Expect(IsOpenFileError(err)).To(BeFalse()) + Expect(IsOpenFileError(fileExistsErr)).To(BeFalse()) + Expect(IsOpenFileError(createDirectoryErr)).To(BeFalse()) + Expect(IsOpenFileError(createFileErr)).To(BeFalse()) + Expect(IsOpenFileError(readFileErr)).To(BeFalse()) + Expect(IsOpenFileError(writeFileErr)).To(BeFalse()) + Expect(IsOpenFileError(closeFileErr)).To(BeFalse()) + }) + }) + Context("IsCreateDirectoryError", func() { It("should return true for create directory errors", func() { Expect(IsCreateDirectoryError(createDirectoryErr)).To(BeTrue()) @@ -170,7 +205,10 @@ var _ = Describe("FileSystem", func() { It("should return false for any other error", func() { Expect(IsCreateDirectoryError(err)).To(BeFalse()) + Expect(IsCreateDirectoryError(fileExistsErr)).To(BeFalse()) + Expect(IsCreateDirectoryError(openFileErr)).To(BeFalse()) Expect(IsCreateDirectoryError(createFileErr)).To(BeFalse()) + Expect(IsCreateDirectoryError(readFileErr)).To(BeFalse()) Expect(IsCreateDirectoryError(writeFileErr)).To(BeFalse()) Expect(IsCreateDirectoryError(closeFileErr)).To(BeFalse()) }) @@ -183,12 +221,31 @@ var _ = Describe("FileSystem", func() { It("should return false for any other error", func() { Expect(IsCreateFileError(err)).To(BeFalse()) + Expect(IsCreateFileError(fileExistsErr)).To(BeFalse()) + Expect(IsCreateFileError(openFileErr)).To(BeFalse()) Expect(IsCreateFileError(createDirectoryErr)).To(BeFalse()) + Expect(IsCreateFileError(readFileErr)).To(BeFalse()) Expect(IsCreateFileError(writeFileErr)).To(BeFalse()) Expect(IsCreateFileError(closeFileErr)).To(BeFalse()) }) }) + Context("IsReadFileError", func() { + It("should return true for read file errors", func() { + Expect(IsReadFileError(readFileErr)).To(BeTrue()) + }) + + It("should return false for any other error", func() { + Expect(IsReadFileError(err)).To(BeFalse()) + Expect(IsReadFileError(fileExistsErr)).To(BeFalse()) + Expect(IsReadFileError(openFileErr)).To(BeFalse()) + Expect(IsReadFileError(createDirectoryErr)).To(BeFalse()) + Expect(IsReadFileError(createFileErr)).To(BeFalse()) + Expect(IsReadFileError(writeFileErr)).To(BeFalse()) + Expect(IsReadFileError(closeFileErr)).To(BeFalse()) + }) + }) + Context("IsWriteFileError", func() { It("should return true for write file errors", func() { Expect(IsWriteFileError(writeFileErr)).To(BeTrue()) @@ -196,8 +253,11 @@ var _ = Describe("FileSystem", func() { It("should return false for any other error", func() { Expect(IsWriteFileError(err)).To(BeFalse()) + Expect(IsWriteFileError(fileExistsErr)).To(BeFalse()) + Expect(IsWriteFileError(openFileErr)).To(BeFalse()) Expect(IsWriteFileError(createDirectoryErr)).To(BeFalse()) Expect(IsWriteFileError(createFileErr)).To(BeFalse()) + Expect(IsWriteFileError(readFileErr)).To(BeFalse()) Expect(IsWriteFileError(closeFileErr)).To(BeFalse()) }) }) @@ -209,22 +269,28 @@ var _ = Describe("FileSystem", func() { It("should return false for any other error", func() { Expect(IsCloseFileError(err)).To(BeFalse()) + Expect(IsCloseFileError(fileExistsErr)).To(BeFalse()) + Expect(IsCloseFileError(openFileErr)).To(BeFalse()) Expect(IsCloseFileError(createDirectoryErr)).To(BeFalse()) Expect(IsCloseFileError(createFileErr)).To(BeFalse()) + Expect(IsCloseFileError(readFileErr)).To(BeFalse()) Expect(IsCloseFileError(writeFileErr)).To(BeFalse()) }) }) Describe("error messages", func() { It("should contain the wrapped err", func() { + Expect(fileExistsErr.Error()).To(ContainSubstring(err.Error())) + Expect(openFileErr.Error()).To(ContainSubstring(err.Error())) Expect(createDirectoryErr.Error()).To(ContainSubstring(err.Error())) Expect(createFileErr.Error()).To(ContainSubstring(err.Error())) + Expect(readFileErr.Error()).To(ContainSubstring(err.Error())) Expect(writeFileErr.Error()).To(ContainSubstring(err.Error())) Expect(closeFileErr.Error()).To(ContainSubstring(err.Error())) }) }) }) - // NOTE: FileSystem.Exists, FileSystem.Create and FileSystem.Create().Write are hard - // to test in unitary tests as they deal with actual files + // NOTE: FileSystem.Exists, FileSystem.Open, FileSystem.Open().Read, FileSystem.Create and FileSystem.Create().Write + // are hard to test in unitary tests as they deal with actual files }) diff --git a/pkg/scaffold/internal/filesystem/mock.go b/pkg/scaffold/internal/filesystem/mock.go index a32f1cb2c39..87cd4c14ac2 100644 --- a/pkg/scaffold/internal/filesystem/mock.go +++ b/pkg/scaffold/internal/filesystem/mock.go @@ -26,8 +26,11 @@ type mockFileSystem struct { path string exists func(path string) bool existsError error + openFileError error createDirError error createFileError error + input *bytes.Buffer + readFileError error output *bytes.Buffer writeFileError error closeFileError error @@ -59,8 +62,7 @@ func MockPath(path string) MockOptions { } } -// MockExists makes FileSystem.Exists use the provided function to check if the -// file exists +// MockExists makes FileSystem.Exists use the provided function to check if the file exists func MockExists(exists func(path string) bool) MockOptions { return func(fs *mockFileSystem) { fs.exists = exists @@ -74,6 +76,13 @@ func MockExistsError(err error) MockOptions { } } +// MockOpenFileError makes FileSystem.Open return err +func MockOpenFileError(err error) MockOptions { + return func(fs *mockFileSystem) { + fs.openFileError = err + } +} + // MockCreateDirError makes FileSystem.Create return err func MockCreateDirError(err error) MockOptions { return func(fs *mockFileSystem) { @@ -88,6 +97,20 @@ func MockCreateFileError(err error) MockOptions { } } +// MockInput provides a buffer where the content will be read from +func MockInput(input *bytes.Buffer) MockOptions { + return func(fs *mockFileSystem) { + fs.input = input + } +} + +// MockReadFileError makes the Read method (of the io.Reader returned by FileSystem.Open) return err +func MockReadFileError(err error) MockOptions { + return func(fs *mockFileSystem) { + fs.readFileError = err + } +} + // MockOutput provides a buffer where the content will be written func MockOutput(output *bytes.Buffer) MockOptions { return func(fs *mockFileSystem) { @@ -95,16 +118,14 @@ func MockOutput(output *bytes.Buffer) MockOptions { } } -// MockWriteFileError makes the Write method (of the io.Writer returned by -// FileSystem.Create) return err +// MockWriteFileError makes the Write method (of the io.Writer returned by FileSystem.Create) return err func MockWriteFileError(err error) MockOptions { return func(fs *mockFileSystem) { fs.writeFileError = err } } -// MockCloseFileError makes the Write method (of the io.Writer returned by -// FileSystem.Create) return err +// MockCloseFileError makes the Write method (of the io.Writer returned by FileSystem.Create) return err func MockCloseFileError(err error) MockOptions { return func(fs *mockFileSystem) { fs.closeFileError = err @@ -114,12 +135,25 @@ func MockCloseFileError(err error) MockOptions { // Exists implements FileSystem.Exists func (fs mockFileSystem) Exists(path string) (bool, error) { if fs.existsError != nil { - return false, fs.existsError + return false, fileExistsError{path, fs.existsError} } return fs.exists(path), nil } +// Open implements FileSystem.Open +func (fs mockFileSystem) Open(path string) (io.ReadCloser, error) { + if fs.openFileError != nil { + return nil, openFileError{path, fs.openFileError} + } + + if fs.input == nil { + fs.input = bytes.NewBufferString("Hello world!") + } + + return &mockReadFile{path, fs.input, fs.readFileError, fs.closeFileError}, nil +} + // Create implements FileSystem.Create func (fs mockFileSystem) Create(path string) (io.Writer, error) { if fs.createDirError != nil { @@ -130,11 +164,37 @@ func (fs mockFileSystem) Create(path string) (io.Writer, error) { return nil, createFileError{path, fs.createFileError} } - return &mockFile{path, fs.output, fs.writeFileError, fs.closeFileError}, nil + return &mockWriteFile{path, fs.output, fs.writeFileError, fs.closeFileError}, nil +} + +// mockReadFile implements io.Reader mocking a readFile for tests +type mockReadFile struct { + path string + input *bytes.Buffer + readFileError error + closeFileError error +} + +// Read implements io.Reader.ReadCloser +func (f *mockReadFile) Read(content []byte) (n int, err error) { + if f.readFileError != nil { + return 0, readFileError{path: f.path, err: f.readFileError} + } + + return f.input.Read(content) +} + +// Read implements io.Reader.ReadCloser +func (f *mockReadFile) Close() error { + if f.closeFileError != nil { + return closeFileError{path: f.path, err: f.closeFileError} + } + + return nil } -// mockFile implements io.Writer mocking a file for tests -type mockFile struct { +// mockWriteFile implements io.Writer mocking a writeFile for tests +type mockWriteFile struct { path string content *bytes.Buffer writeFileError error @@ -142,7 +202,7 @@ type mockFile struct { } // Write implements io.Writer.Write -func (f *mockFile) Write(content []byte) (n int, err error) { +func (f *mockWriteFile) Write(content []byte) (n int, err error) { defer func() { if err == nil && f.closeFileError != nil { err = closeFileError{path: f.path, err: f.closeFileError} diff --git a/pkg/scaffold/internal/filesystem/mock_test.go b/pkg/scaffold/internal/filesystem/mock_test.go index 131c537527f..a01f943e72d 100644 --- a/pkg/scaffold/internal/filesystem/mock_test.go +++ b/pkg/scaffold/internal/filesystem/mock_test.go @@ -31,6 +31,7 @@ func TestMockFileSystem(t *testing.T) { RunSpecs(t, "MockFileSystem suite") } +//nolint:dupl var _ = Describe("MockFileSystem", func() { var ( fsi FileSystem @@ -60,6 +61,14 @@ var _ = Describe("MockFileSystem", func() { Expect(exists).To(BeFalse()) }) + It("should open readable files", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Read([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + It("should create writable files", func() { f, err := fsi.Create("") Expect(err).NotTo(HaveOccurred()) @@ -86,6 +95,14 @@ var _ = Describe("MockFileSystem", func() { Expect(exists).To(BeFalse()) }) + It("should open readable files", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Read([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + It("should create writable files", func() { f, err := fsi.Create("") Expect(err).NotTo(HaveOccurred()) @@ -114,6 +131,14 @@ var _ = Describe("MockFileSystem", func() { Expect(exists).To(BeTrue()) }) + It("should open readable files", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Read([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + It("should create writable files", func() { f, err := fsi.Create("") Expect(err).NotTo(HaveOccurred()) @@ -135,7 +160,45 @@ var _ = Describe("MockFileSystem", func() { It("should error when calling Exists", func() { _, err := fsi.Exists("") Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(testErr.Error())) + Expect(IsFileExistsError(err)).To(BeTrue()) + }) + + It("should open readable files", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Read([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should create writable files", func() { + f, err := fsi.Create("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Write([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Context("when using MockOpenFileError", func() { + BeforeEach(func() { + options = []MockOptions{MockOpenFileError(testErr)} + }) + + It("should be a mockFileSystem instance", func() { + Expect(ok).To(BeTrue()) + }) + + It("should claim that files don't exist", func() { + exists, err := fsi.Exists("") + Expect(err).NotTo(HaveOccurred()) + Expect(exists).To(BeFalse()) + }) + + It("should error when calling Open", func() { + _, err := fsi.Open("") + Expect(err).To(HaveOccurred()) + Expect(IsOpenFileError(err)).To(BeTrue()) }) It("should create writable files", func() { @@ -162,6 +225,14 @@ var _ = Describe("MockFileSystem", func() { Expect(exists).To(BeFalse()) }) + It("should open readable files", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Read([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + It("should error when calling Create", func() { _, err := fsi.Create("") Expect(err).To(HaveOccurred()) @@ -169,7 +240,7 @@ var _ = Describe("MockFileSystem", func() { }) }) - Context("when using MockCreateFileError", func() { // TODO + Context("when using MockCreateFileError", func() { BeforeEach(func() { options = []MockOptions{MockCreateFileError(testErr)} }) @@ -184,6 +255,14 @@ var _ = Describe("MockFileSystem", func() { Expect(exists).To(BeFalse()) }) + It("should open readable files", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Read([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + It("should error when calling Create", func() { _, err := fsi.Create("") Expect(err).To(HaveOccurred()) @@ -191,6 +270,81 @@ var _ = Describe("MockFileSystem", func() { }) }) + Context("when using MockInput", func() { + var ( + input *bytes.Buffer + fileContent = []byte("Hello world!") + ) + + BeforeEach(func() { + input = bytes.NewBufferString("Hello world!") + options = []MockOptions{MockInput(input)} + }) + + It("should be a mockFileSystem instance", func() { + Expect(ok).To(BeTrue()) + }) + + It("should claim that files don't exist", func() { + exists, err := fsi.Exists("") + Expect(err).NotTo(HaveOccurred()) + Expect(exists).To(BeFalse()) + }) + + It("should open readable files and the content to be accessible", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + output := make([]byte, len(fileContent)) + n, err := f.Read(output) + Expect(err).NotTo(HaveOccurred()) + Expect(n).To(Equal(len(fileContent))) + Expect(output).To(Equal(fileContent)) + }) + + It("should create writable files", func() { + f, err := fsi.Create("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Write([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Context("when using MockReadFileError", func() { + BeforeEach(func() { + options = []MockOptions{MockReadFileError(testErr)} + }) + + It("should be a mockFileSystem instance", func() { + Expect(ok).To(BeTrue()) + }) + + It("should claim that files don't exist", func() { + exists, err := fsi.Exists("") + Expect(err).NotTo(HaveOccurred()) + Expect(exists).To(BeFalse()) + }) + + It("should error when calling Open().Read", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + output := make([]byte, 0) + _, err = f.Read(output) + Expect(err).To(HaveOccurred()) + Expect(IsReadFileError(err)).To(BeTrue()) + }) + + It("should create writable files", func() { + f, err := fsi.Create("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Write([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + }) + Context("when using MockOutput", func() { var ( output bytes.Buffer @@ -212,6 +366,14 @@ var _ = Describe("MockFileSystem", func() { Expect(exists).To(BeFalse()) }) + It("should open readable files", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Read([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + It("should create writable files and the content should be accesible", func() { f, err := fsi.Create("") Expect(err).NotTo(HaveOccurred()) @@ -238,6 +400,14 @@ var _ = Describe("MockFileSystem", func() { Expect(exists).To(BeFalse()) }) + It("should open readable files", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + _, err = f.Read([]byte("")) + Expect(err).NotTo(HaveOccurred()) + }) + It("should error when calling Create().Write", func() { f, err := fsi.Create("") Expect(err).NotTo(HaveOccurred()) @@ -263,6 +433,15 @@ var _ = Describe("MockFileSystem", func() { Expect(exists).To(BeFalse()) }) + It("should error when calling Open().Close", func() { + f, err := fsi.Open("") + Expect(err).NotTo(HaveOccurred()) + + err = f.Close() + Expect(err).To(HaveOccurred()) + Expect(IsCloseFileError(err)).To(BeTrue()) + }) + It("should error when calling Create().Write", func() { f, err := fsi.Create("") Expect(err).NotTo(HaveOccurred()) diff --git a/pkg/scaffold/internal/machinery/scaffold.go b/pkg/scaffold/internal/machinery/scaffold.go index 7cae7738f8f..22cc377399e 100644 --- a/pkg/scaffold/internal/machinery/scaffold.go +++ b/pkg/scaffold/internal/machinery/scaffold.go @@ -17,8 +17,10 @@ limitations under the License. package machinery import ( + "bufio" "bytes" "fmt" + "io/ioutil" "path/filepath" "strings" "text/template" @@ -39,8 +41,8 @@ var options = imports.Options{ // Scaffold uses templates to scaffold new files type Scaffold interface { - // Execute writes to disk the provided templates - Execute(*model.Universe, ...file.Template) error + // Execute writes to disk the provided files + Execute(*model.Universe, ...file.Builder) error } // scaffold implements Scaffold interface @@ -60,26 +62,49 @@ func NewScaffold(plugins ...model.Plugin) Scaffold { } // Execute implements Scaffold.Execute -func (s *scaffold) Execute(universe *model.Universe, files ...file.Template) error { +func (s *scaffold) Execute(universe *model.Universe, files ...file.Builder) error { + // Initialize the universe files + universe.Files = make(map[string]*file.File, len(files)) + // Set the repo as the local prefix so that it knows how to group imports if universe.Config != nil { imports.LocalPrefix = universe.Config.Repo } for _, f := range files { - m, err := buildFileModel(universe, f) - if err != nil { - return err + // Inject common fields + universe.InjectInto(f) + + // Validate file builders + if reqValFile, requiresValidation := f.(file.RequiresValidation); requiresValidation { + if err := reqValFile.Validate(); err != nil { + return err + } + } + + // Build models for Template builders + if t, isTemplate := f.(file.Template); isTemplate { + if err := s.buildFileModel(t, universe.Files); err != nil { + return err + } + } + + // Build models for Inserter builders + if i, isInserter := f.(file.Inserter); isInserter { + if err := s.updateFileModel(i, universe.Files); err != nil { + return err + } } - universe.Files = append(universe.Files, m) } + // Execute plugins for _, plugin := range s.plugins { if err := plugin.Pipe(universe); err != nil { return err } } + // Persist the files to disk for _, f := range universe.Files { if err := s.writeFile(f); err != nil { return err @@ -90,64 +115,39 @@ func (s *scaffold) Execute(universe *model.Universe, files ...file.Template) err } // buildFileModel scaffolds a single file -func buildFileModel(universe *model.Universe, t file.Template) (*file.File, error) { - // Inject common fields - universe.InjectInto(t) - - // Validate the file scaffold - if reqValFile, ok := t.(file.RequiresValidation); ok { - if err := reqValFile.Validate(); err != nil { - return nil, err - } - } - - // Get the template input params +func (scaffold) buildFileModel(t file.Template, models map[string]*file.File) error { + // Set the template default values err := t.SetTemplateDefaults() - if err != nil { - return nil, err - } - - m := &file.File{ - Path: t.GetPath(), - IfExistsAction: t.GetIfExistsAction(), - } - - b, err := doTemplate(t) - if err != nil { - return nil, err - } - m.Contents = string(b) - - return m, nil -} - -func (s *scaffold) writeFile(f *file.File) error { - // Check if the file to write already exists - exists, err := s.fs.Exists(f.Path) if err != nil { return err } - if exists { - switch f.IfExistsAction { - case file.Overwrite: - // By not returning, the file is written as if it didn't exist + + // Handle already existing models + if _, found := models[t.GetPath()]; found { + switch t.GetIfExistsAction() { case file.Skip: - // By returning nil, the file is not written but the process will carry on return nil case file.Error: - // By returning an error, the file is not written and the process will fail - return fmt.Errorf("failed to create %s: file already exists", f.Path) + return fmt.Errorf("failed to create %s: model already exists", t.GetPath()) + case file.Overwrite: + default: + return fmt.Errorf("unknown behavior if file exists (%d) for %s", t.GetIfExistsAction(), t.GetPath()) } } - writer, err := s.fs.Create(f.Path) + m := &file.File{ + Path: t.GetPath(), + IfExistsAction: t.GetIfExistsAction(), + } + + b, err := doTemplate(t) if err != nil { return err } + m.Contents = string(b) - _, err = writer.Write([]byte(f.Contents)) - - return err + models[m.Path] = m + return nil } // doTemplate executes the template for a file using the input @@ -164,6 +164,7 @@ func doTemplate(t file.Template) ([]byte, error) { } b := out.Bytes() + // TODO(adirio): move go-formatting to write step // gofmt the imports if filepath.Ext(t.GetPath()) == ".go" { b, err = imports.Process(t.GetPath(), b, &options) @@ -182,3 +183,204 @@ func newTemplate(t file.Template) *template.Template { "lower": strings.ToLower, }) } + +// updateFileModel updates a single file +func (s scaffold) updateFileModel(i file.Inserter, models map[string]*file.File) error { + m, err := s.loadPreviousModel(i, models) + if err != nil { + return err + } + + // Get valid code fragments + codeFragments := getValidCodeFragments(i) + + // Remove code fragments that already were applied + err = filterExistingValues(m.Contents, codeFragments) + if err != nil { + return err + } + + // If no code fragment to insert, we are done + if len(codeFragments) == 0 { + return nil + } + + content, err := insertStrings(m.Contents, codeFragments) + if err != nil { + return err + } + + // TODO(adirio): move go-formatting to write step + formattedContent := content + if ext := filepath.Ext(i.GetPath()); ext == ".go" { + formattedContent, err = imports.Process(i.GetPath(), content, nil) + if err != nil { + return err + } + } + + m.Contents = string(formattedContent) + m.IfExistsAction = file.Overwrite + models[m.Path] = m + return nil +} + +// loadPreviousModel gets the previous model from the models map or the actual file +func (s scaffold) loadPreviousModel(i file.Inserter, models map[string]*file.File) (*file.File, error) { + // Lets see if we already have a model for this file + if m, found := models[i.GetPath()]; found { + // Check if there is already an scaffolded file + exists, err := s.fs.Exists(i.GetPath()) + if err != nil { + return nil, err + } + + // If there is a model but no scaffolded file we return the model + if !exists { + return m, nil + } + + // If both a model and a file are found, check which has preference + switch m.IfExistsAction { + case file.Skip: + // File has preference + fromFile, err := s.loadModelFromFile(i.GetPath()) + if err != nil { + return m, nil + } + return fromFile, nil + case file.Error: + // Writing will result in an error, so we can return error now + return nil, fmt.Errorf("failed to create %s: file already exists", i.GetPath()) + case file.Overwrite: + // Model has preference + return m, nil + default: + return nil, fmt.Errorf("unknown behavior if file exists (%d) for %s", m.IfExistsAction, i.GetPath()) + } + } + + // There was no model + return s.loadModelFromFile(i.GetPath()) +} + +// loadModelFromFile gets the previous model from the actual file +func (s scaffold) loadModelFromFile(path string) (f *file.File, err error) { + reader, err := s.fs.Open(path) + if err != nil { + return + } + defer func() { + closeErr := reader.Close() + if err == nil { + err = closeErr + } + }() + + content, err := ioutil.ReadAll(reader) + if err != nil { + return + } + + f = &file.File{Path: path, Contents: string(content)} + return +} + +// getValidCodeFragments obtains the code fragments from a file.Inserter +func getValidCodeFragments(i file.Inserter) file.CodeFragmentsMap { + // Get the code fragments + codeFragments := i.GetCodeFragments() + + // Validate the code fragments + validMarkers := i.GetMarkers() + for marker := range codeFragments { + valid := false + for _, validMarker := range validMarkers { + if marker == validMarker { + valid = true + break + } + } + if !valid { + delete(codeFragments, marker) + } + } + + return codeFragments +} + +// filterExistingValues removes the single-line values that already exists +// TODO: Add support for multi-line duplicate values +func filterExistingValues(content string, codeFragmentsMap file.CodeFragmentsMap) error { + scanner := bufio.NewScanner(strings.NewReader(content)) + for scanner.Scan() { + line := scanner.Text() + for marker, codeFragments := range codeFragmentsMap { + for i, codeFragment := range codeFragments { + if strings.TrimSpace(line) == strings.TrimSpace(codeFragment) { + codeFragmentsMap[marker] = append(codeFragments[:i], codeFragments[i+1:]...) + } + } + if len(codeFragmentsMap[marker]) == 0 { + delete(codeFragmentsMap, marker) + } + } + } + if err := scanner.Err(); err != nil { + return err + } + return nil +} + +func insertStrings(content string, codeFragmentsMap file.CodeFragmentsMap) ([]byte, error) { + out := new(bytes.Buffer) + + scanner := bufio.NewScanner(strings.NewReader(content)) + for scanner.Scan() { + line := scanner.Text() + + for marker, codeFragments := range codeFragmentsMap { + if strings.TrimSpace(line) == strings.TrimSpace(marker.String()) { + for _, codeFragment := range codeFragments { + _, _ = out.WriteString(codeFragment) // bytes.Buffer.WriteString always returns nil errors + } + } + } + + _, _ = out.WriteString(line + "\n") // bytes.Buffer.WriteString always returns nil errors + } + if err := scanner.Err(); err != nil { + return nil, err + } + + return out.Bytes(), nil +} + +func (s scaffold) writeFile(f *file.File) error { + // Check if the file to write already exists + exists, err := s.fs.Exists(f.Path) + if err != nil { + return err + } + if exists { + switch f.IfExistsAction { + case file.Overwrite: + // By not returning, the file is written as if it didn't exist + case file.Skip: + // By returning nil, the file is not written but the process will carry on + return nil + case file.Error: + // By returning an error, the file is not written and the process will fail + return fmt.Errorf("failed to create %s: file already exists", f.Path) + } + } + + writer, err := s.fs.Create(f.Path) + if err != nil { + return err + } + + _, err = writer.Write([]byte(f.Contents)) + + return err +} diff --git a/pkg/scaffold/internal/machinery/scaffold_test.go b/pkg/scaffold/internal/machinery/scaffold_test.go index c79e46b3139..066986cc038 100644 --- a/pkg/scaffold/internal/machinery/scaffold_test.go +++ b/pkg/scaffold/internal/machinery/scaffold_test.go @@ -20,10 +20,10 @@ import ( "testing" . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" "sigs.k8s.io/kubebuilder/pkg/model" - "sigs.k8s.io/kubebuilder/pkg/model/config" "sigs.k8s.io/kubebuilder/pkg/model/file" "sigs.k8s.io/kubebuilder/pkg/scaffold/internal/filesystem" ) @@ -103,157 +103,267 @@ var _ = Describe("Scaffold", func() { const fileContent = "Hello world!" var ( - s Scaffold - output bytes.Buffer - testError = errors.New("error text") + output bytes.Buffer + testErr = errors.New("error text") ) BeforeEach(func() { output.Reset() }) - It("should write the file", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockOutput(&output), - ), - } + DescribeTable("successes", + func(expected string, files ...file.Builder) { + s := &scaffold{ + fs: filesystem.NewMock( + filesystem.MockOutput(&output), + ), + } - Expect(s.Execute( - model.NewUniverse( - model.WithConfig(&config.Config{}), - ), - fakeFile{ - body: fileContent, - }, - )).To(Succeed()) - Expect(output.String()).To(Equal(fileContent)) - }) + Expect(s.Execute(model.NewUniverse(), files...)).To(Succeed()) + Expect(output.String()).To(Equal(expected)) + }, + Entry("should write the file", + fileContent, + fakeTemplate{body: fileContent}, + ), + Entry("should skip optional models if already have one", + fileContent, + fakeTemplate{body: fileContent}, + fakeTemplate{}, + ), + Entry("should overwrite required models if already have one", + fileContent, + fakeTemplate{}, + fakeTemplate{fakeBuilder: fakeBuilder{ifExistsAction: file.Overwrite}, body: fileContent}, + ), + Entry("should format a go file", + "package file\n", + fakeTemplate{fakeBuilder: fakeBuilder{path: "file.go"}, body: "package file"}, + ), + ) - It("should fail if a plugin fails", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockOutput(&output), - ), - plugins: []model.Plugin{fakePlugin{err: testError}}, - } + DescribeTable("file builders related errors", + func(errMsg string, files ...file.Builder) { + s := &scaffold{fs: filesystem.NewMock()} - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent, - }, - ) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(testError.Error())) - }) + err := s.Execute(model.NewUniverse(), files...) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(errMsg)) + }, + Entry("should fail if unable to validate a file builder", + testErr.Error(), + fakeRequiresValidation{validateErr: testErr}, + ), + Entry("should fail if unable to set default values for a template", + testErr.Error(), + fakeTemplate{err: testErr}, + ), + Entry("should fail if an unexpected previous model is found", + "failed to create filename: model already exists", + fakeTemplate{fakeBuilder: fakeBuilder{path: "filename"}}, + fakeTemplate{fakeBuilder: fakeBuilder{path: "filename", ifExistsAction: file.Error}}, + ), + Entry("should fail if behavior if file exists is not defined", + "unknown behavior if file exists (-1) for filename", + fakeTemplate{fakeBuilder: fakeBuilder{path: "filename"}}, + fakeTemplate{fakeBuilder: fakeBuilder{path: "filename", ifExistsAction: -1}}, + ), + Entry("should fail if a template is broken", + "template: ", + fakeTemplate{body: "{{ .Field }"}, + ), + Entry("should fail if a template params aren't provided", + "template: ", + fakeTemplate{body: "{{ .Field }}"}, + ), + Entry("should fail if unable to format a go file", + "expected 'package', found ", + fakeTemplate{fakeBuilder: fakeBuilder{path: "file.go"}, body: fileContent}, + ), + ) - It("should fail if a template validation fails", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockOutput(&output), - ), - } + DescribeTable("insert strings", + func(input, expected string, files ...file.Builder) { + s := &scaffold{ + fs: filesystem.NewMock( + filesystem.MockInput(bytes.NewBufferString(input)), + filesystem.MockOutput(&output), + filesystem.MockExists(func(_ string) bool { return len(input) != 0 }), + ), + } - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent, - validateError: testError, + Expect(s.Execute(model.NewUniverse(), files...)).To(Succeed()) + Expect(output.String()).To(Equal(expected)) + }, + Entry("should insert lines for go files", + ` +// +kubebuilder:scaffold:- +`, + ` +1 +2 +// +kubebuilder:scaffold:- +`, + fakeInserter{codeFragments: file.CodeFragmentsMap{ + file.NewMarkerFor("file.go", "-"): {"1\n", "2\n"}}, }, - ) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(testError.Error())) - }) - - It("should fail if a template SetTemplateDefaults method fails", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockOutput(&output), - ), - } - - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent, - err: testError, + ), + Entry("should insert lines for yaml files", + ` +# +kubebuilder:scaffold:- +`, + ` +1 +2 +# +kubebuilder:scaffold:- +`, + fakeInserter{codeFragments: file.CodeFragmentsMap{ + file.NewMarkerFor("file.yaml", "-"): {"1\n", "2\n"}}, }, - ) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(testError.Error())) - }) - - It("should fail if a template is broken", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockOutput(&output), - ), - } - - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent + "{{ .Field }", + ), + Entry("should use models if there is no file", + "", + ` +1 +2 +// +kubebuilder:scaffold:- +`, + fakeTemplate{fakeBuilder: fakeBuilder{ifExistsAction: file.Overwrite}, body: ` +// +kubebuilder:scaffold:- +`}, + fakeInserter{codeFragments: file.CodeFragmentsMap{ + file.NewMarkerFor("file.go", "-"): {"1\n", "2\n"}}, }, - ) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("template: ")) - }) - - It("should fail if a template params aren't provided", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockOutput(&output), - ), - } - - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent + "{{ .Field }}", + ), + Entry("should use required models over files", + fileContent, + ` +1 +2 +// +kubebuilder:scaffold:- +`, + fakeTemplate{fakeBuilder: fakeBuilder{ifExistsAction: file.Overwrite}, body: ` +// +kubebuilder:scaffold:- +`}, + fakeInserter{codeFragments: file.CodeFragmentsMap{ + file.NewMarkerFor("file.go", "-"): {"1\n", "2\n"}}, }, - ) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("template: ")) - }) + ), + Entry("should use files over optional models", + ` +// +kubebuilder:scaffold:- +`, + ` +1 +2 +// +kubebuilder:scaffold:- +`, + fakeTemplate{body: fileContent}, + fakeInserter{ + codeFragments: file.CodeFragmentsMap{ + file.NewMarkerFor("file.go", "-"): {"1\n", "2\n"}, + }, + }, + ), + Entry("should filter invalid markers", + ` +// +kubebuilder:scaffold:- +// +kubebuilder:scaffold:* +`, + ` +1 +2 +// +kubebuilder:scaffold:- +// +kubebuilder:scaffold:* +`, + fakeInserter{ + markers: []file.Marker{file.NewMarkerFor("file.go", "-")}, + codeFragments: file.CodeFragmentsMap{ + file.NewMarkerFor("file.go", "-"): {"1\n", "2\n"}, + file.NewMarkerFor("file.go", "*"): {"3\n", "4\n"}, + }, + }, + ), + Entry("should filter already existing one-line code fragments", + ` +1 +// +kubebuilder:scaffold:- +3 +4 +// +kubebuilder:scaffold:* +`, + ` +1 +2 +// +kubebuilder:scaffold:- +3 +4 +// +kubebuilder:scaffold:* +`, + fakeInserter{ + codeFragments: file.CodeFragmentsMap{ + file.NewMarkerFor("file.go", "-"): {"1\n", "2\n"}, + file.NewMarkerFor("file.go", "*"): {"3\n", "4\n"}, + }, + }, + ), + Entry("should not insert anything if no code fragment", + "", // input is provided through a template as mock fs doesn't copy it to the output buffer if no-op + ` +// +kubebuilder:scaffold:- +`, + fakeTemplate{body: ` +// +kubebuilder:scaffold:- +`}, + fakeInserter{ + codeFragments: file.CodeFragmentsMap{ + file.NewMarkerFor("file.go", "-"): {}, + }, + }, + ), + ) - It("should format a go file", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockOutput(&output), - ), - } + DescribeTable("insert strings related errors", + func(errMsg string, files ...file.Builder) { + s := &scaffold{ + fs: filesystem.NewMock( + filesystem.MockExists(func(_ string) bool { return true }), + ), + } - Expect(s.Execute( - model.NewUniverse(), - fakeFile{ - path: "file.go", - body: "package file", - }, - )).To(Succeed()) - Expect(output.String()).To(Equal("package file\n")) - }) + err := s.Execute(model.NewUniverse(), files...) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(errMsg)) + }, + Entry("should fail if inserting into a model that fails when a file exists and it does exist", + "failed to create filename: file already exists", + fakeTemplate{fakeBuilder: fakeBuilder{path: "filename", ifExistsAction: file.Error}}, + fakeInserter{fakeBuilder: fakeBuilder{path: "filename"}}, + ), + Entry("should fail if inserting into a model with unknown behavior if the file exists and it does exist", + "unknown behavior if file exists (-1) for filename", + fakeTemplate{fakeBuilder: fakeBuilder{path: "filename", ifExistsAction: -1}}, + fakeInserter{fakeBuilder: fakeBuilder{path: "filename"}}, + ), + ) - It("should fail if unable to format a go file", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockOutput(&output), - ), + It("should fail if a plugin fails", func() { + s := &scaffold{ + fs: filesystem.NewMock(), + plugins: []model.Plugin{fakePlugin{err: testErr}}, } err := s.Execute( model.NewUniverse(), - fakeFile{ - path: "file.go", - body: fileContent, - }, + fakeTemplate{}, ) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("expected 'package', found ")) + Expect(err.Error()).To(ContainSubstring(testErr.Error())) }) - Context("when the file already exists", func() { + Context("write when the file already exists", func() { + var s Scaffold + BeforeEach(func() { s = &scaffold{ fs: filesystem.NewMock( @@ -266,9 +376,7 @@ var _ = Describe("Scaffold", func() { It("should skip the file by default", func() { Expect(s.Execute( model.NewUniverse(), - fakeFile{ - body: fileContent, - }, + fakeTemplate{body: fileContent}, )).To(Succeed()) Expect(output.String()).To(BeEmpty()) }) @@ -276,10 +384,7 @@ var _ = Describe("Scaffold", func() { It("should write the file if configured to do so", func() { Expect(s.Execute( model.NewUniverse(), - fakeFile{ - body: fileContent, - ifExistsAction: file.Overwrite, - }, + fakeTemplate{fakeBuilder: fakeBuilder{ifExistsAction: file.Overwrite}, body: fileContent}, )).To(Succeed()) Expect(output.String()).To(Equal(fileContent)) }) @@ -287,11 +392,7 @@ var _ = Describe("Scaffold", func() { It("should error if configured to do so", func() { err := s.Execute( model.NewUniverse(), - fakeFile{ - path: "filename", - body: fileContent, - ifExistsAction: file.Error, - }, + fakeTemplate{fakeBuilder: fakeBuilder{path: "filename", ifExistsAction: file.Error}, body: fileContent}, ) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("failed to create filename: file already exists")) @@ -299,93 +400,60 @@ var _ = Describe("Scaffold", func() { }) }) - Context("when the filesystem returns an error", func() { - - It("should fail if fs.Exists failed", func() { - s = &scaffold{ + DescribeTable("filesystem errors", + func( + mockErrorF func(error) filesystem.MockOptions, + checkErrorF func(error) bool, + files ...file.Builder, + ) { + s := &scaffold{ fs: filesystem.NewMock( - filesystem.MockExistsError(testError), + mockErrorF(testErr), ), } - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent, - }, - ) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(testError.Error())) - }) - - It("should fail if fs.Create was unable to create the directory", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockCreateDirError(testError), - ), - } - - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent, - }, - ) - Expect(err).To(HaveOccurred()) - Expect(filesystem.IsCreateDirectoryError(err)).To(BeTrue()) - }) - - It("should fail if fs.Create was unable to create the file", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockCreateFileError(testError), - ), - } - - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent, - }, - ) - Expect(err).To(HaveOccurred()) - Expect(filesystem.IsCreateFileError(err)).To(BeTrue()) - }) - - It("should fail if fs.Create().Write was unable to write the file", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockWriteFileError(testError), - ), - } - - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent, - }, - ) - Expect(err).To(HaveOccurred()) - Expect(filesystem.IsWriteFileError(err)).To(BeTrue()) - }) - - It("should fail if fs.Create().Write was unable to close the file", func() { - s = &scaffold{ - fs: filesystem.NewMock( - filesystem.MockCloseFileError(testError), - ), - } - - err := s.Execute( - model.NewUniverse(), - fakeFile{ - body: fileContent, - }, - ) + err := s.Execute(model.NewUniverse(), files...) Expect(err).To(HaveOccurred()) - Expect(filesystem.IsCloseFileError(err)).To(BeTrue()) - }) - }) + Expect(checkErrorF(err)).To(BeTrue()) + }, + Entry("should fail if fs.Exists failed (at file writing)", + filesystem.MockExistsError, filesystem.IsFileExistsError, + fakeTemplate{}, + ), + Entry("should fail if fs.Exists failed (at model updating)", + filesystem.MockExistsError, filesystem.IsFileExistsError, + fakeTemplate{}, + fakeInserter{}, + ), + Entry("should fail if fs.Open was unable to open the file", + filesystem.MockOpenFileError, filesystem.IsOpenFileError, + fakeInserter{}, + ), + Entry("should fail if fs.Open().Read was unable to read the file", + filesystem.MockReadFileError, filesystem.IsReadFileError, + fakeInserter{}, + ), + Entry("should fail if fs.Open().Close was unable to close the file", + filesystem.MockCloseFileError, filesystem.IsCloseFileError, + fakeInserter{}, + ), + Entry("should fail if fs.Create was unable to create the directory", + filesystem.MockCreateDirError, filesystem.IsCreateDirectoryError, + fakeTemplate{}, + ), + Entry("should fail if fs.Create was unable to create the file", + filesystem.MockCreateFileError, filesystem.IsCreateFileError, + fakeTemplate{}, + ), + Entry("should fail if fs.Create().Write was unable to write the file", + filesystem.MockWriteFileError, filesystem.IsWriteFileError, + fakeTemplate{}, + ), + Entry("should fail if fs.Create().Write was unable to close the file", + filesystem.MockCloseFileError, filesystem.IsCloseFileError, + fakeTemplate{}, + ), + ) }) }) @@ -401,35 +469,55 @@ func (f fakePlugin) Pipe(_ *model.Universe) error { return f.err } -var _ file.Template = fakeFile{} +var _ file.Builder = fakeBuilder{} -// fakeFile is used to mock a file.File in order to test Scaffold -type fakeFile struct { +// fakeBuilder is used to mock a file.Builder +type fakeBuilder struct { path string - body string ifExistsAction file.IfExistsAction - - err error - validateError error } -// GetPath implements file.Template -func (f fakeFile) GetPath() string { +// GetPath implements file.Builder +func (f fakeBuilder) GetPath() string { return f.path } -// GetBody implements file.Template -func (f fakeFile) GetBody() string { - return f.body +// GetIfExistsAction implements file.Builder +func (f fakeBuilder) GetIfExistsAction() file.IfExistsAction { + return f.ifExistsAction } -// GetIfExistsAction implements file.Template -func (f fakeFile) GetIfExistsAction() file.IfExistsAction { - return f.ifExistsAction +var _ file.RequiresValidation = fakeRequiresValidation{} + +// fakeRequiresValidation is used to mock a file.RequiresValidation in order to test Scaffold +type fakeRequiresValidation struct { + fakeBuilder + + validateErr error +} + +// Validate implements file.RequiresValidation +func (f fakeRequiresValidation) Validate() error { + return f.validateErr +} + +var _ file.Template = fakeTemplate{} + +// fakeTemplate is used to mock a file.File in order to test Scaffold +type fakeTemplate struct { + fakeBuilder + + body string + err error +} + +// GetBody implements file.Template +func (f fakeTemplate) GetBody() string { + return f.body } // SetTemplateDefaults implements file.Template -func (f fakeFile) SetTemplateDefaults() error { +func (f fakeTemplate) SetTemplateDefaults() error { if f.err != nil { return f.err } @@ -437,7 +525,27 @@ func (f fakeFile) SetTemplateDefaults() error { return nil } -// Validate implements file.RequiresValidation -func (f fakeFile) Validate() error { - return f.validateError +type fakeInserter struct { + fakeBuilder + + markers []file.Marker + codeFragments file.CodeFragmentsMap +} + +// GetMarkers implements file.UpdatableTemplate +func (f fakeInserter) GetMarkers() []file.Marker { + if f.markers != nil { + return f.markers + } + + markers := make([]file.Marker, 0, len(f.codeFragments)) + for marker := range f.codeFragments { + markers = append(markers, marker) + } + return markers +} + +// GetCodeFragments implements file.UpdatableTemplate +func (f fakeInserter) GetCodeFragments() file.CodeFragmentsMap { + return f.codeFragments } diff --git a/pkg/scaffold/internal/machinery/string_utils.go b/pkg/scaffold/internal/machinery/string_utils.go deleted file mode 100644 index be14bcda81c..00000000000 --- a/pkg/scaffold/internal/machinery/string_utils.go +++ /dev/null @@ -1,134 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -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 - - http://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 machinery - -import ( - "bufio" - "bytes" - "io" - "io/ioutil" - "os" - "path/filepath" - "strings" - - "golang.org/x/tools/imports" -) - -// insertStrings reads content from given reader and insert string below the -// line containing marker string. So for ex. in insertStrings(r, {'m1': -// [v1], 'm2': [v2]}) -// v1 will be inserted below the lines containing m1 string and v2 will be inserted -// below line containing m2 string. -func insertStrings(r io.Reader, markerAndValues map[string][]string) (io.Reader, error) { - // reader clone is needed since we will be reading twice from the given reader - buf := new(bytes.Buffer) - rClone := io.TeeReader(r, buf) - - err := filterExistingValues(rClone, markerAndValues) - if err != nil { - return nil, err - } - - out := new(bytes.Buffer) - - scanner := bufio.NewScanner(buf) - for scanner.Scan() { - line := scanner.Text() - - for marker, vals := range markerAndValues { - if strings.TrimSpace(line) == strings.TrimSpace(marker) { - for _, val := range vals { - _, err := out.WriteString(val) - if err != nil { - return nil, err - } - } - } - } - _, err := out.WriteString(line + "\n") - if err != nil { - return nil, err - } - } - if err := scanner.Err(); err != nil { - return nil, err - } - return out, nil -} - -func InsertStringsInFile(path string, markerAndValues map[string][]string) error { - isGoFile := false - if ext := filepath.Ext(path); ext == ".go" { - isGoFile = true - } - - f, err := os.Open(path) - if err != nil { - return err - } - - r, err := insertStrings(f, markerAndValues) - if err != nil { - return err - } - - err = f.Close() - if err != nil { - return err - } - - content, err := ioutil.ReadAll(r) - if err != nil { - return err - } - - formattedContent := content - if isGoFile { - formattedContent, err = imports.Process(path, content, nil) - if err != nil { - return err - } - } - - // use Go import process to format the content - err = ioutil.WriteFile(path, formattedContent, os.ModePerm) - if err != nil { - return err - } - - return err -} - -// filterExistingValues removes the single-line values that already exists in -// the given reader. Multi-line values are ignore currently simply because we -// don't have a use-case for it. -func filterExistingValues(r io.Reader, markerAndValues map[string][]string) error { - scanner := bufio.NewScanner(r) - for scanner.Scan() { - line := scanner.Text() - for marker, vals := range markerAndValues { - for i, val := range vals { - if strings.TrimSpace(line) == strings.TrimSpace(val) { - markerAndValues[marker] = append(vals[:i], vals[i+1:]...) - } - } - } - } - if err := scanner.Err(); err != nil { - return err - } - return nil -} diff --git a/pkg/scaffold/internal/machinery/string_utils_test.go b/pkg/scaffold/internal/machinery/string_utils_test.go deleted file mode 100644 index 2f60786805a..00000000000 --- a/pkg/scaffold/internal/machinery/string_utils_test.go +++ /dev/null @@ -1,101 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -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 - - http://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 machinery - -import ( - "bytes" - "io/ioutil" - "testing" -) - -type insertStrTest struct { - input string - markerNValues map[string][]string - expected string -} - -func TestInsertStrBelowMarker(t *testing.T) { - - tests := []insertStrTest{ - { - input: ` -v1beta1.AddToScheme(scheme) -// +kubebuilder:scaffold:apis-add-scheme -`, - markerNValues: map[string][]string{ - "// +kubebuilder:scaffold:apis-add-scheme": { - "v1.AddToScheme(scheme)\n", "somefunc()\n", - }, - }, - expected: ` -v1beta1.AddToScheme(scheme) -v1.AddToScheme(scheme) -somefunc() -// +kubebuilder:scaffold:apis-add-scheme -`, - }, - { // avoid duplicates - input: ` -v1beta1.AddToScheme(scheme) -// +kubebuilder:scaffold:apis-add-scheme -`, - markerNValues: map[string][]string{ - "// +kubebuilder:scaffold:apis-add-scheme": { - "v1beta1.AddToScheme(scheme)\n", "v1.AddToScheme(scheme)\n", - }, - }, - expected: ` -v1beta1.AddToScheme(scheme) -v1.AddToScheme(scheme) -// +kubebuilder:scaffold:apis-add-scheme -`, - }, - { - // string with literal format - input: ` -v1beta1.AddToScheme(scheme) -// +kubebuilder:scaffold:apis-add-scheme -`, - markerNValues: map[string][]string{ - "// +kubebuilder:scaffold:apis-add-scheme": { - `v1.AddToScheme(scheme) -`, - }, - }, - expected: ` -v1beta1.AddToScheme(scheme) -v1.AddToScheme(scheme) -// +kubebuilder:scaffold:apis-add-scheme -`, - }, - } - - for _, test := range tests { - result, err := insertStrings(bytes.NewBufferString(test.input), test.markerNValues) - if err != nil { - t.Errorf("error %v", err) - } - - b, err := ioutil.ReadAll(result) - if err != nil { - t.Errorf("error: %v", err) - } - - if string(b) != test.expected { - t.Errorf("got: %s and wanted: %s", string(b), test.expected) - } - } -} diff --git a/pkg/scaffold/internal/templates/v1/manager/apis.go b/pkg/scaffold/internal/templates/v1/manager/apis.go index c0c4a64bb33..9e7c7936181 100644 --- a/pkg/scaffold/internal/templates/v1/manager/apis.go +++ b/pkg/scaffold/internal/templates/v1/manager/apis.go @@ -30,6 +30,7 @@ var _ file.Template = &APIs{} type APIs struct { file.TemplateMixin file.BoilerplateMixin + // BoilerplatePath is the path to the boilerplate file BoilerplatePath string // Comments is a list of comments to add to the apis.go diff --git a/pkg/scaffold/internal/templates/v1/manager/config.go b/pkg/scaffold/internal/templates/v1/manager/config.go index fecbe9867a7..bee783ced28 100644 --- a/pkg/scaffold/internal/templates/v1/manager/config.go +++ b/pkg/scaffold/internal/templates/v1/manager/config.go @@ -27,6 +27,7 @@ var _ file.Template = &Config{} // Config scaffolds yaml config for the manager. type Config struct { file.TemplateMixin + // Image is controller manager image name Image string } diff --git a/pkg/scaffold/internal/templates/v2/controller/controller_suitetest.go b/pkg/scaffold/internal/templates/v2/controller/controller_suitetest.go index 08c4d24cb31..8cf1e1883ac 100644 --- a/pkg/scaffold/internal/templates/v2/controller/controller_suitetest.go +++ b/pkg/scaffold/internal/templates/v2/controller/controller_suitetest.go @@ -21,11 +21,10 @@ import ( "path/filepath" "sigs.k8s.io/kubebuilder/pkg/model/file" - "sigs.k8s.io/kubebuilder/pkg/scaffold/internal/machinery" - templatesv2 "sigs.k8s.io/kubebuilder/pkg/scaffold/internal/templates/v2" ) var _ file.Template = &SuiteTest{} +var _ file.Inserter = &SuiteTest{} // SuiteTest scaffolds the suite_test.go file to setup the controller test type SuiteTest struct { @@ -36,7 +35,7 @@ type SuiteTest struct { file.ResourceMixin } -// SetTemplateDefaults implements input.Template +// SetTemplateDefaults implements file.Template func (f *SuiteTest) SetTemplateDefaults() error { if f.Path == "" { if f.MultiGroup { @@ -46,7 +45,10 @@ func (f *SuiteTest) SetTemplateDefaults() error { } } - f.TemplateBody = controllerSuiteTestTemplate + f.TemplateBody = fmt.Sprintf(controllerSuiteTestTemplate, + file.NewMarkerFor(f.Path, importMarker), + file.NewMarkerFor(f.Path, addSchemeMarker), + ) return nil } @@ -56,6 +58,51 @@ func (f *SuiteTest) Validate() error { return f.Resource.Validate() } +const ( + importMarker = "imports" + addSchemeMarker = "scheme" +) + +// GetMarkers implements file.Inserter +func (f *SuiteTest) GetMarkers() []file.Marker { + return []file.Marker{ + file.NewMarkerFor(f.Path, importMarker), + file.NewMarkerFor(f.Path, addSchemeMarker), + } +} + +const ( + apiImportCodeFragment = `%s "%s" +` + addschemeCodeFragment = `err = %s.AddToScheme(scheme.Scheme) +Expect(err).NotTo(HaveOccurred()) + +` +) + +// GetCodeFragments implements file.Inserter +func (f *SuiteTest) GetCodeFragments() file.CodeFragmentsMap { + fragments := make(file.CodeFragmentsMap, 2) + + // Generate import code fragments + imports := make([]string, 0) + imports = append(imports, fmt.Sprintf(apiImportCodeFragment, f.Resource.ImportAlias, f.Resource.Package)) + + // Generate add scheme code fragments + addScheme := make([]string, 0) + addScheme = append(addScheme, fmt.Sprintf(addschemeCodeFragment, f.Resource.ImportAlias)) + + // Only store code fragments in the map if the slices are non-empty + if len(imports) != 0 { + fragments[file.NewMarkerFor(f.Path, importMarker)] = imports + } + if len(addScheme) != 0 { + fragments[file.NewMarkerFor(f.Path, addSchemeMarker)] = addScheme + } + + return fragments +} + const controllerSuiteTestTemplate = `{{ .Boilerplate }} package controllers @@ -71,7 +118,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" - // +kubebuilder:scaffold:imports + %s ) // These tests use Ginkgo (BDD-style Go testing framework). Refer to @@ -102,7 +149,7 @@ var _ = BeforeSuite(func(done Done) { Expect(err).ToNot(HaveOccurred()) Expect(cfg).ToNot(BeNil()) - // +kubebuilder:scaffold:scheme + %s k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) Expect(err).ToNot(HaveOccurred()) @@ -117,28 +164,3 @@ var _ = AfterSuite(func() { Expect(err).ToNot(HaveOccurred()) }) ` - -// Update updates given file (suite_test.go) with code fragments required for -// adding import paths and code setup for new types. -func (f *SuiteTest) Update() error { - ctrlImportCodeFragment := fmt.Sprintf(`"%s/controllers" -`, f.Repo) - apiImportCodeFragment := fmt.Sprintf(`%s "%s" -`, f.Resource.ImportAlias, f.Resource.Package) - - addschemeCodeFragment := fmt.Sprintf(`err = %s.AddToScheme(scheme.Scheme) -Expect(err).NotTo(HaveOccurred()) - -`, f.Resource.ImportAlias) - - err := machinery.InsertStringsInFile(f.Path, - map[string][]string{ - templatesv2.APIPkgImportScaffoldMarker: {ctrlImportCodeFragment, apiImportCodeFragment}, - templatesv2.APISchemeScaffoldMarker: {addschemeCodeFragment}, - }) - if err != nil { - return err - } - - return nil -} diff --git a/pkg/scaffold/internal/templates/v2/crd/kustomization.go b/pkg/scaffold/internal/templates/v2/crd/kustomization.go index c0015442d54..54bd07d5d4c 100644 --- a/pkg/scaffold/internal/templates/v2/crd/kustomization.go +++ b/pkg/scaffold/internal/templates/v2/crd/kustomization.go @@ -21,16 +21,10 @@ import ( "path/filepath" "sigs.k8s.io/kubebuilder/pkg/model/file" - "sigs.k8s.io/kubebuilder/pkg/scaffold/internal/machinery" -) - -const ( - kustomizeResourceScaffoldMarker = "# +kubebuilder:scaffold:crdkustomizeresource" - kustomizeWebhookPatchScaffoldMarker = "# +kubebuilder:scaffold:crdkustomizewebhookpatch" - kustomizeCAInjectionPatchScaffoldMarker = "# +kubebuilder:scaffold:crdkustomizecainjectionpatch" ) var _ file.Template = &Kustomization{} +var _ file.Inserter = &Kustomization{} // Kustomization scaffolds the kustomization file in manager folder. type Kustomization struct { @@ -38,38 +32,76 @@ type Kustomization struct { file.ResourceMixin } -// SetTemplateDefaults implements input.Template +// SetTemplateDefaults implements file.Template func (f *Kustomization) SetTemplateDefaults() error { if f.Path == "" { f.Path = filepath.Join("config", "crd", "kustomization.yaml") } - f.TemplateBody = kustomizationTemplate + f.TemplateBody = fmt.Sprintf(kustomizationTemplate, + file.NewMarkerFor(f.Path, resourceMarker), + file.NewMarkerFor(f.Path, webhookPatchMarker), + file.NewMarkerFor(f.Path, caInjectionPatchMarker), + ) return nil } -func (f *Kustomization) Update() error { - if f.Path == "" { - f.Path = filepath.Join("config", "crd", "kustomization.yaml") +const ( + resourceMarker = "crdkustomizeresource" + webhookPatchMarker = "crdkustomizewebhookpatch" + caInjectionPatchMarker = "crdkustomizecainjectionpatch" +) + +// GetMarkers implements file.Inserter +func (f *Kustomization) GetMarkers() []file.Marker { + return []file.Marker{ + file.NewMarkerFor(f.Path, resourceMarker), + file.NewMarkerFor(f.Path, webhookPatchMarker), + file.NewMarkerFor(f.Path, caInjectionPatchMarker), } +} - // TODO(directxman12): not technically valid if something changes from the default - // (we'd need to parse the markers) +const ( + resourceCodeFragment = `- bases/%s_%s.yaml +` + webhookPatchCodeFragment = `#- patches/webhook_in_%s.yaml +` + caInjectionPatchCodeFragment = `#- patches/cainjection_in_%s.yaml +` +) + +// GetCodeFragments implements file.Inserter +func (f *Kustomization) GetCodeFragments() file.CodeFragmentsMap { + fragments := make(file.CodeFragmentsMap, 3) + + // Generate resource code fragments + res := make([]string, 0) + res = append(res, fmt.Sprintf(resourceCodeFragment, f.Resource.Domain, f.Resource.Plural)) + + // Generate resource code fragments + webhookPatch := make([]string, 0) + webhookPatch = append(webhookPatch, fmt.Sprintf(webhookPatchCodeFragment, f.Resource.Plural)) - kustomizeResourceCodeFragment := fmt.Sprintf("- bases/%s_%s.yaml\n", f.Resource.Domain, f.Resource.Plural) - kustomizeWebhookPatchCodeFragment := fmt.Sprintf("#- patches/webhook_in_%s.yaml\n", f.Resource.Plural) - kustomizeCAInjectionPatchCodeFragment := fmt.Sprintf("#- patches/cainjection_in_%s.yaml\n", f.Resource.Plural) + // Generate resource code fragments + caInjectionPatch := make([]string, 0) + caInjectionPatch = append(caInjectionPatch, fmt.Sprintf(caInjectionPatchCodeFragment, f.Resource.Plural)) + + // Only store code fragments in the map if the slices are non-empty + if len(res) != 0 { + fragments[file.NewMarkerFor(f.Path, resourceMarker)] = res + } + if len(webhookPatch) != 0 { + fragments[file.NewMarkerFor(f.Path, webhookPatchMarker)] = webhookPatch + } + if len(caInjectionPatch) != 0 { + fragments[file.NewMarkerFor(f.Path, caInjectionPatchMarker)] = caInjectionPatch + } - return machinery.InsertStringsInFile(f.Path, - map[string][]string{ - kustomizeResourceScaffoldMarker: {kustomizeResourceCodeFragment}, - kustomizeWebhookPatchScaffoldMarker: {kustomizeWebhookPatchCodeFragment}, - kustomizeCAInjectionPatchScaffoldMarker: {kustomizeCAInjectionPatchCodeFragment}, - }) + return fragments } -var kustomizationTemplate = fmt.Sprintf(`# This kustomization.yaml is not intended to be run by itself, +var kustomizationTemplate = `# This kustomization.yaml is not intended to be run by itself, # since it depends on service name and namespace that are out of this kustomize package. # It should be run by config/default resources: @@ -87,4 +119,4 @@ patchesStrategicMerge: # the following config is for teaching kustomize how to do kustomization for CRDs. configurations: - kustomizeconfig.yaml -`, kustomizeResourceScaffoldMarker, kustomizeWebhookPatchScaffoldMarker, kustomizeCAInjectionPatchScaffoldMarker) +` diff --git a/pkg/scaffold/internal/templates/v2/main.go b/pkg/scaffold/internal/templates/v2/main.go index ae30d032084..4169b90414f 100644 --- a/pkg/scaffold/internal/templates/v2/main.go +++ b/pkg/scaffold/internal/templates/v2/main.go @@ -20,57 +20,83 @@ import ( "fmt" "path/filepath" - "sigs.k8s.io/kubebuilder/pkg/model/config" "sigs.k8s.io/kubebuilder/pkg/model/file" - "sigs.k8s.io/kubebuilder/pkg/model/resource" - "sigs.k8s.io/kubebuilder/pkg/scaffold/internal/machinery" ) -const ( - APIPkgImportScaffoldMarker = "// +kubebuilder:scaffold:imports" - APISchemeScaffoldMarker = "// +kubebuilder:scaffold:scheme" - ReconcilerSetupScaffoldMarker = "// +kubebuilder:scaffold:builder" -) +const defaultMainPath = "main.go" var _ file.Template = &Main{} -// Main scaffolds a main.go to run Controllers type Main struct { file.TemplateMixin file.BoilerplateMixin } -// SetTemplateDefaults implements input.Template +// SetTemplateDefaults implements file.Template func (f *Main) SetTemplateDefaults() error { if f.Path == "" { - f.Path = filepath.Join("main.go") + f.Path = filepath.Join(defaultMainPath) } - f.TemplateBody = mainTemplate + f.TemplateBody = fmt.Sprintf(mainTemplate, + file.NewMarkerFor(f.Path, importMarker), + file.NewMarkerFor(f.Path, addSchemeMarker), + file.NewMarkerFor(f.Path, setupMarker), + ) return nil } -// Update updates main.go with code fragments required to wire a new -// resource/controller. -func (f *Main) Update(opts *MainUpdateOptions) error { - path := "main.go" +var _ file.Inserter = &MainUpdater{} - // generate all the code fragments - apiImportCodeFragment := fmt.Sprintf(`%s "%s" -`, opts.Resource.ImportAlias, opts.Resource.Package) +// MainUpdater updates main.go to run Controllers +type MainUpdater struct { //nolint:maligned + file.RepositoryMixin + file.MultiGroupMixin + file.ResourceMixin + + // Flags to indicate which parts need to be included when updating the file + WireResource, WireController, WireWebhook bool +} - addschemeCodeFragment := fmt.Sprintf(`_ = %s.AddToScheme(scheme) -`, opts.Resource.ImportAlias) +// GetPath implements Builder +func (*MainUpdater) GetPath() string { + return defaultMainPath +} - var reconcilerSetupCodeFragment, ctrlImportCodeFragment string +// GetPath implements Builder +func (*MainUpdater) GetIfExistsAction() file.IfExistsAction { + return file.Overwrite +} - if opts.Config.MultiGroup { +const ( + importMarker = "imports" + addSchemeMarker = "scheme" + setupMarker = "builder" +) - ctrlImportCodeFragment = fmt.Sprintf(`%scontroller "%s/controllers/%s" -`, opts.Resource.GroupPackageName, opts.Config.Repo, opts.Resource.Group) +// GetMarkers implements file.Inserter +func (f *MainUpdater) GetMarkers() []file.Marker { + return []file.Marker{ + file.NewMarkerFor(defaultMainPath, importMarker), + file.NewMarkerFor(defaultMainPath, addSchemeMarker), + file.NewMarkerFor(defaultMainPath, setupMarker), + } +} - reconcilerSetupCodeFragment = fmt.Sprintf(`if err = (&%scontroller.%sReconciler{ +const ( + apiImportCodeFragment = `%s "%s" +` + controllerImportCodeFragment = `"%s/controllers" +` + // TODO(v3): `&%scontrollers` should be used instead of `&%scontroller` as there may be multiple + // controller for different Kinds in the same group. However, this is a backwards incompatible + // change, and thus should be done for next project version. + multiGroupControllerImportCodeFragment = `%scontroller "%s/controllers/%s" +` + addschemeCodeFragment = `_ = %s.AddToScheme(scheme) +` + reconcilerSetupCodeFragment = `if err = (&controllers.%sReconciler{ Client: mgr.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("%s"), Scheme: mgr.GetScheme(), @@ -78,79 +104,82 @@ func (f *Main) Update(opts *MainUpdateOptions) error { setupLog.Error(err, "unable to create controller", "controller", "%s") os.Exit(1) } -`, opts.Resource.GroupPackageName, opts.Resource.Kind, opts.Resource.Kind, opts.Resource.Kind) - - } else { - - ctrlImportCodeFragment = fmt.Sprintf(`"%s/controllers" -`, opts.Config.Repo) - - reconcilerSetupCodeFragment = fmt.Sprintf(`if err = (&controllers.%sReconciler{ +` + // TODO(v3): loggers for the same Kind controllers from different groups use the same logger. + // `.WithName("controllers").WithName(GROUP).WithName(KIND)` should be used instead. However, + // this is a backwards incompatible change, and thus should be done for next project version. + multiGroupReconcilerSetupCodeFragment = `if err = (&%scontroller.%sReconciler{ Client: mgr.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("%s"), - Scheme: mgr.GetScheme(), + Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "%s") os.Exit(1) } -`, opts.Resource.Kind, opts.Resource.Kind, opts.Resource.Kind) - - } - - webhookSetupCodeFragment := fmt.Sprintf(`if err = (&%s.%s{}).SetupWebhookWithManager(mgr); err != nil { +` + webhookSetupCodeFragment = `if err = (&%s.%s{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "%s") os.Exit(1) } -`, opts.Resource.ImportAlias, opts.Resource.Kind, opts.Resource.Kind) - - if opts.WireResource { - err := machinery.InsertStringsInFile(path, - map[string][]string{ - APIPkgImportScaffoldMarker: {apiImportCodeFragment}, - APISchemeScaffoldMarker: {addschemeCodeFragment}, - }) - if err != nil { - return err - } - } +` +) - if opts.WireController { - return machinery.InsertStringsInFile(path, - map[string][]string{ - APIPkgImportScaffoldMarker: {apiImportCodeFragment, ctrlImportCodeFragment}, - APISchemeScaffoldMarker: {addschemeCodeFragment}, - ReconcilerSetupScaffoldMarker: {reconcilerSetupCodeFragment}, - }) - } +// GetCodeFragments implements file.Inserter +func (f *MainUpdater) GetCodeFragments() file.CodeFragmentsMap { + fragments := make(file.CodeFragmentsMap, 3) - if opts.WireWebhook { - return machinery.InsertStringsInFile(path, - map[string][]string{ - APIPkgImportScaffoldMarker: {apiImportCodeFragment, ctrlImportCodeFragment}, - APISchemeScaffoldMarker: {addschemeCodeFragment}, - ReconcilerSetupScaffoldMarker: {webhookSetupCodeFragment}, - }) + // If resource is not being provided we are creating the file, not updating it + if f.Resource == nil { + return fragments } - return nil -} + // Generate import code fragments + imports := make([]string, 0) + imports = append(imports, fmt.Sprintf(apiImportCodeFragment, f.Resource.ImportAlias, f.Resource.Package)) + if f.WireController { + if !f.MultiGroup { + imports = append(imports, fmt.Sprintf(controllerImportCodeFragment, f.Repo)) + } else { + imports = append(imports, fmt.Sprintf(multiGroupControllerImportCodeFragment, + f.Resource.GroupPackageName, f.Repo, f.Resource.Group)) + } + } -// MainUpdateOptions contains info required for wiring an API/Controller in -// main.go. -type MainUpdateOptions struct { - // Config contains info about the project - Config *config.Config + // Generate add scheme code fragments + addScheme := make([]string, 0) + addScheme = append(addScheme, fmt.Sprintf(addschemeCodeFragment, f.Resource.ImportAlias)) + + // Generate setup code fragments + setup := make([]string, 0) + if f.WireController { + if !f.MultiGroup { + setup = append(setup, fmt.Sprintf(reconcilerSetupCodeFragment, + f.Resource.Kind, f.Resource.Kind, f.Resource.Kind)) + } else { + setup = append(setup, fmt.Sprintf(multiGroupReconcilerSetupCodeFragment, + f.Resource.GroupPackageName, f.Resource.Kind, f.Resource.Kind, f.Resource.Kind)) + } + } + if f.WireWebhook { + setup = append(setup, fmt.Sprintf(webhookSetupCodeFragment, + f.Resource.ImportAlias, f.Resource.Kind, f.Resource.Kind)) + } - // Resource is the resource being added - Resource *resource.Resource + // Only store code fragments in the map if the slices are non-empty + if len(imports) != 0 { + fragments[file.NewMarkerFor(defaultMainPath, importMarker)] = imports + } + if len(addScheme) != 0 { + fragments[file.NewMarkerFor(defaultMainPath, addSchemeMarker)] = addScheme + } + if len(setup) != 0 { + fragments[file.NewMarkerFor(defaultMainPath, setupMarker)] = setup + } - // Flags to indicate if resource/controller is being scaffolded or not - WireResource bool - WireController bool - WireWebhook bool + return fragments } -var mainTemplate = fmt.Sprintf(`{{ .Boilerplate }} +var mainTemplate = `{{ .Boilerplate }} package main @@ -208,4 +237,4 @@ func main() { os.Exit(1) } } -`, APIPkgImportScaffoldMarker, APISchemeScaffoldMarker, ReconcilerSetupScaffoldMarker) +` diff --git a/pkg/scaffold/internal/templates/v2/makefile.go b/pkg/scaffold/internal/templates/v2/makefile.go index fe22691e006..e29f5bb03cb 100644 --- a/pkg/scaffold/internal/templates/v2/makefile.go +++ b/pkg/scaffold/internal/templates/v2/makefile.go @@ -25,6 +25,7 @@ var _ file.Template = &Makefile{} // Makefile scaffolds the Makefile type Makefile struct { file.TemplateMixin + // Image is controller manager image name Image string // BoilerplatePath is the path to the boilerplate file diff --git a/pkg/scaffold/internal/templates/v2/manager/config.go b/pkg/scaffold/internal/templates/v2/manager/config.go index f037cbbb2c0..73efeac6dff 100644 --- a/pkg/scaffold/internal/templates/v2/manager/config.go +++ b/pkg/scaffold/internal/templates/v2/manager/config.go @@ -27,6 +27,7 @@ var _ file.Template = &Config{} // Config scaffolds yaml config for the manager. type Config struct { file.TemplateMixin + // Image is controller manager image name Image string } diff --git a/pkg/scaffold/webhook.go b/pkg/scaffold/webhook.go index 2c37abf9cd8..9ff6454d390 100644 --- a/pkg/scaffold/webhook.go +++ b/pkg/scaffold/webhook.go @@ -92,15 +92,19 @@ func (s *webhookScaffolder) Scaffold() error { } } +func (s *webhookScaffolder) newUniverse() *model.Universe { + return model.NewUniverse( + model.WithConfig(s.config), + model.WithBoilerplate(s.boilerplate), + model.WithResource(s.resource), + ) +} + func (s *webhookScaffolder) scaffoldV1() error { webhookConfig := webhookv1.Config{Server: s.server, Type: s.webhookType, Operations: s.operations} return machinery.NewScaffold().Execute( - model.NewUniverse( - model.WithConfig(s.config), - model.WithBoilerplate(s.boilerplate), - model.WithResource(s.resource), - ), + s.newUniverse(), &managerv1.Webhook{}, &webhookv1.AdmissionHandler{Config: webhookConfig}, &webhookv1.AdmissionWebhookBuilder{Config: webhookConfig}, @@ -125,29 +129,13 @@ func (s *webhookScaffolder) scaffoldV2() error { You need to implement the conversion.Hub and conversion.Convertible interfaces for your CRD types.`) } - webhookScaffolder := &webhookv2.Webhook{Defaulting: s.defaulting, Validating: s.validation} if err := machinery.NewScaffold().Execute( - model.NewUniverse( - model.WithConfig(s.config), - model.WithBoilerplate(s.boilerplate), - model.WithResource(s.resource), - ), - webhookScaffolder, + s.newUniverse(), + &webhookv2.Webhook{Defaulting: s.defaulting, Validating: s.validation}, + &templatesv2.MainUpdater{WireWebhook: true}, ); err != nil { return err } - if err := (&templatesv2.Main{}).Update( - &templatesv2.MainUpdateOptions{ - Config: s.config, - WireResource: false, - WireController: false, - WireWebhook: true, - Resource: s.resource, - }, - ); err != nil { - return fmt.Errorf("error updating main.go: %v", err) - } - return nil } diff --git a/plugins/addon/helpers.go b/plugins/addon/helpers.go index 5bff964076c..99034f81e3c 100644 --- a/plugins/addon/helpers.go +++ b/plugins/addon/helpers.go @@ -28,13 +28,11 @@ func AddFile(u *model.Universe, add *file.File) (bool, error) { return false, fmt.Errorf("path must be set") } - for _, f := range u.Files { - if f.Path == p { - return false, nil - } + if _, found := u.Files[p]; found { + return false, nil } - u.Files = append(u.Files, add) + u.Files[p] = add return true, nil } @@ -46,11 +44,9 @@ func ReplaceFileIfExists(u *model.Universe, add *file.File) bool { panic("path must be set") } - for i, f := range u.Files { - if f.Path == p { - u.Files[i] = add - return true - } + if _, found := u.Files[p]; found { + u.Files[p] = add + return true } return false