From f26b1ef6fcd7eb9074096d9ed8f793b0b6394e10 Mon Sep 17 00:00:00 2001 From: Zorian Motso Date: Thu, 11 Apr 2024 12:06:28 +0300 Subject: [PATCH] feat: Add the ability to remove the files with GerritMergeRequest CR (#30) Change-Id: If9b6963174f52d8015d5e1e857c31adb73674830 (cherry picked from commit 5f9738cf2dcdf7ec2022ef8e2da21fc3dc159b7a) --- .golangci.yaml | 10 ---- Dockerfile | 2 +- api/v1/gerritmergerequest_types.go | 4 +- api/v1/zz_generated.deepcopy.go | 1 - api/v1alpha1/zz_generated.deepcopy.go | 1 - .../v2.edp.epam.com_gerritmergerequests.yaml | 8 +-- controllers/merge_request/controller.go | 24 +++++++-- controllers/merge_request/controller_test.go | 23 +++++++-- .../v2.edp.epam.com_gerritmergerequests.yaml | 8 +-- docs/api.md | 2 +- mock/gerrit/mock_git_client.go | 20 ++++++++ pkg/client/git/client.go | 16 ++++++ pkg/client/git/client_test.go | 49 +++++++++++++++++++ sonar-project.properties | 2 +- 14 files changed, 138 insertions(+), 32 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 480e407..3de978e 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -428,15 +428,6 @@ linters-settings: enable-all: false disable-all: false - depguard: - list-type: blacklist - include-go-root: false - packages: - - github.com/sirupsen/logrus - packages-with-error-message: - # specify an error message to output when a blacklisted package is used - - github.com/sirupsen/logrus: "logging is allowed only by logutils.Log" - ifshort: # Maximum length of variable declaration measured in number of lines, after which linter won't suggest using short syntax. # Has higher priority than max-decl-chars. @@ -882,7 +873,6 @@ linters: - containedctx - cyclop - decorder - - depguard - dogsled - dupl - durationcheck diff --git a/Dockerfile b/Dockerfile index 153f20d..85a3eb9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -7,7 +7,7 @@ ENV OPERATOR=/usr/local/bin/gerrit-operator \ RUN apk add --no-cache ca-certificates==20240226-r0 \ openssh-client==9.3_p2-r1 \ - openssl==3.1.4-r5 \ + openssl==3.1.4-r6 \ git==2.40.1-r0 # install operator binary diff --git a/api/v1/gerritmergerequest_types.go b/api/v1/gerritmergerequest_types.go index 532fd19..14e8ca9 100644 --- a/api/v1/gerritmergerequest_types.go +++ b/api/v1/gerritmergerequest_types.go @@ -49,7 +49,9 @@ type GerritMergeRequestSpec struct { CommitMessage string `json:"commitMessage,omitempty"` // ChangesConfigMap is the name of the ConfigMap, which contains files contents that should be merged. - // ConfigMap should eny data keys with content in the json format: {"path": "/controllers/user.go", "contents": "some code here"}. + // ConfigMap should contain eny data keys with content in the json + // format: {"path": "/controllers/user.go", "contents": "some code here"} - to add file + // or format: {"path": "/controllers/user.go"} - to remove file. // If files already exist in the project, they will be overwritten. // If empty, sourceBranch should be set. // +optional diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 4b4edb5..34ba822 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated // Code generated by controller-gen. DO NOT EDIT. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 44c60db..7b5cb82 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated // Code generated by controller-gen. DO NOT EDIT. diff --git a/config/crd/bases/v2.edp.epam.com_gerritmergerequests.yaml b/config/crd/bases/v2.edp.epam.com_gerritmergerequests.yaml index 20ef926..01807a9 100644 --- a/config/crd/bases/v2.edp.epam.com_gerritmergerequests.yaml +++ b/config/crd/bases/v2.edp.epam.com_gerritmergerequests.yaml @@ -55,9 +55,11 @@ spec: changesConfigMap: description: 'ChangesConfigMap is the name of the ConfigMap, which contains files contents that should be merged. ConfigMap should - eny data keys with content in the json format: {"path": "/controllers/user.go", - "contents": "some code here"}. If files already exist in the project, - they will be overwritten. If empty, sourceBranch should be set.' + contain eny data keys with content in the json format: {"path": + "/controllers/user.go", "contents": "some code here"} - to add file + or format: {"path": "/controllers/user.go"} - to remove file. If + files already exist in the project, they will be overwritten. If + empty, sourceBranch should be set.' example: config-map-new-feature type: string commitMessage: diff --git a/controllers/merge_request/controller.go b/controllers/merge_request/controller.go index 368d168..4c1cb86 100644 --- a/controllers/merge_request/controller.go +++ b/controllers/merge_request/controller.go @@ -57,6 +57,7 @@ type GitClient interface { CheckoutBranch(projectName, branch string) error Commit(projectName, message string, files []string, user *git.User) error SetFileContents(projectName, filePath, contents string) error + RemoveFile(projectName, filePath string) (bool, error) } type GerritClient interface { @@ -65,8 +66,8 @@ type GerritClient interface { } type MRConfigMapFile struct { - Path string `json:"path"` - Contents string `json:"contents"` + Path string `json:"path"` + Contents *string `json:"contents"` } func NewReconcile(k8sClient client.Client, log logr.Logger, @@ -302,11 +303,24 @@ func (r *Reconcile) commitFiles(ctx context.Context, instance *gerritApi.GerritM return errors.Wrap(err, "unable to decode file") } - if err := gitClient.SetFileContents(instance.Spec.ProjectName, mrFile.Path, mrFile.Contents); err != nil { - return errors.Wrap(err, "unable to set file contents") + fileChanged := true + + if mrFile.Contents != nil { + if err := gitClient.SetFileContents(instance.Spec.ProjectName, mrFile.Path, *mrFile.Contents); err != nil { + return errors.Wrap(err, "unable to set file contents") + } + } else { + var err error + + fileChanged, err = gitClient.RemoveFile(instance.Spec.ProjectName, mrFile.Path) + if err != nil { + return errors.Wrap(err, "unable to remove file") + } } - addFiles = append(addFiles, mrFile.Path) + if fileChanged { + addFiles = append(addFiles, mrFile.Path) + } } gitUser := &git.User{Name: instance.Spec.AuthorName, Email: instance.Spec.AuthorEmail} diff --git a/controllers/merge_request/controller_test.go b/controllers/merge_request/controller_test.go index 9af3eee..33b9a0b 100644 --- a/controllers/merge_request/controller_test.go +++ b/controllers/merge_request/controller_test.go @@ -9,6 +9,7 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" coreV1 "k8s.io/api/core/v1" @@ -228,9 +229,15 @@ func (s *ControllerTestSuite) TestConfigMap() { err := coreV1.AddToScheme(s.scheme) require.NoError(s.T(), err) - cm := coreV1.ConfigMap{Data: map[string]string{"test.txt": `{"path": "test.txt", "contents": "test"}`}, ObjectMeta: metaV1.ObjectMeta{ - Name: s.mergeRequest.Spec.ChangesConfigMap, Namespace: s.mergeRequest.Namespace, - }} + cm := coreV1.ConfigMap{ + ObjectMeta: metaV1.ObjectMeta{ + Name: s.mergeRequest.Spec.ChangesConfigMap, Namespace: s.mergeRequest.Namespace, + }, + Data: map[string]string{ + "create file": `{"path": "test.txt", "contents": "test"}`, + "remove file": `{"path": "test2.txt"}`, + }, + } fakeClient := fake.NewClientBuilder().WithScheme(s.scheme). WithRuntimeObjects(s.rootGerrit, s.mergeRequest, &cm).Build() @@ -243,8 +250,14 @@ func (s *ControllerTestSuite) TestConfigMap() { s.gitClient.On("CheckoutBranch", s.mergeRequest.Spec.ProjectName, s.mergeRequest.TargetBranch()). Return(nil) s.gitClient.On("SetFileContents", s.mergeRequest.Spec.ProjectName, "test.txt", "test").Return(nil) - s.gitClient.On("Commit", s.mergeRequest.Spec.ProjectName, commitMessage(s.mergeRequest.CommitMessage(), - changeID), []string{"test.txt"}, + s.gitClient.On("RemoveFile", s.mergeRequest.Spec.ProjectName, "test2.txt").Return(true, nil) + s.gitClient.On("Commit", + s.mergeRequest.Spec.ProjectName, + commitMessage(s.mergeRequest.CommitMessage(), changeID), + mock.MatchedBy(func(files []string) bool { + return assert.Contains(s.T(), files, "test.txt") && + assert.Contains(s.T(), files, "test2.txt") + }), &git.User{Name: s.mergeRequest.Spec.AuthorName, Email: s.mergeRequest.Spec.AuthorEmail}).Return(nil) rec := Reconcile{ diff --git a/deploy-templates/crds/v2.edp.epam.com_gerritmergerequests.yaml b/deploy-templates/crds/v2.edp.epam.com_gerritmergerequests.yaml index 20ef926..01807a9 100644 --- a/deploy-templates/crds/v2.edp.epam.com_gerritmergerequests.yaml +++ b/deploy-templates/crds/v2.edp.epam.com_gerritmergerequests.yaml @@ -55,9 +55,11 @@ spec: changesConfigMap: description: 'ChangesConfigMap is the name of the ConfigMap, which contains files contents that should be merged. ConfigMap should - eny data keys with content in the json format: {"path": "/controllers/user.go", - "contents": "some code here"}. If files already exist in the project, - they will be overwritten. If empty, sourceBranch should be set.' + contain eny data keys with content in the json format: {"path": + "/controllers/user.go", "contents": "some code here"} - to add file + or format: {"path": "/controllers/user.go"} - to remove file. If + files already exist in the project, they will be overwritten. If + empty, sourceBranch should be set.' example: config-map-new-feature type: string commitMessage: diff --git a/docs/api.md b/docs/api.md index c017a3f..24cbbbb 100644 --- a/docs/api.md +++ b/docs/api.md @@ -400,7 +400,7 @@ GerritMergeRequestSpec defines the desired state of GerritMergeRequest. changesConfigMap string - ChangesConfigMap is the name of the ConfigMap, which contains files contents that should be merged. ConfigMap should eny data keys with content in the json format: {"path": "/controllers/user.go", "contents": "some code here"}. If files already exist in the project, they will be overwritten. If empty, sourceBranch should be set.
+ ChangesConfigMap is the name of the ConfigMap, which contains files contents that should be merged. ConfigMap should contain eny data keys with content in the json format: {"path": "/controllers/user.go", "contents": "some code here"} - to add file or format: {"path": "/controllers/user.go"} - to remove file. If files already exist in the project, they will be overwritten. If empty, sourceBranch should be set.
false diff --git a/mock/gerrit/mock_git_client.go b/mock/gerrit/mock_git_client.go index b2c2389..0732d80 100644 --- a/mock/gerrit/mock_git_client.go +++ b/mock/gerrit/mock_git_client.go @@ -146,6 +146,26 @@ func (_m *GitClient) SetFileContents(projectName string, filePath string, conten return r0 } +func (_m *GitClient) RemoveFile(projectName, filePath string) (bool, error) { + ret := _m.Called(projectName, filePath) + + var r0 bool + if rf, ok := ret.Get(0).(func(string, string) bool); ok { + r0 = rf(projectName, filePath) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(projectName, filePath) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // SetProjectUser provides a mock function with given fields: projectName, user func (_m *GitClient) SetProjectUser(projectName string, user *git.User) error { ret := _m.Called(projectName, user) diff --git a/pkg/client/git/client.go b/pkg/client/git/client.go index 8ddcd49..342171e 100644 --- a/pkg/client/git/client.go +++ b/pkg/client/git/client.go @@ -66,6 +66,22 @@ func (c *Client) SetFileContents(projectName, filePath, contents string) error { return nil } +func (c *Client) RemoveFile(projectName, filePath string) (bool, error) { + projectPath := c.projectPath(projectName) + filePath = path.Join(projectPath, filePath) + + err := os.Remove(filePath) + if err != nil { + if !os.IsNotExist(err) { + return false, errors.Wrapf(err, "unable to remove file: %s", filePath) + } + + return false, nil + } + + return true, nil +} + func (c *Client) Clone(projectName string) (projectPath string, err error) { projectPath = c.projectPath(projectName) _, err = git.PlainClone( diff --git a/pkg/client/git/client_test.go b/pkg/client/git/client_test.go index 39c3abb..595aaf2 100644 --- a/pkg/client/git/client_test.go +++ b/pkg/client/git/client_test.go @@ -204,3 +204,52 @@ func removeAllWithErrCapture(t *testing.T, p string) { err := os.RemoveAll(p) require.NoError(t, err) } + +func TestClient_RemoveFile(t *testing.T) { + t.Parallel() + + tmp := t.TempDir() + projectDir := path.Join(tmp, "test") + require.NoError(t, os.MkdirAll(projectDir, 0o777)) + + tests := []struct { + name string + filePath string + prepare func(t *testing.T) + want bool + wantErr require.ErrorAssertionFunc + }{ + { + name: "file exists", + filePath: "test.txt", + prepare: func(t *testing.T) { + _, err := os.Create(path.Join(projectDir, "test.txt")) + require.NoError(t, err) + }, + want: true, + wantErr: require.NoError, + }, + { + name: "file does not exist", + filePath: "test.txt", + prepare: func(t *testing.T) { + }, + want: false, + wantErr: require.NoError, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.prepare(t) + + c := &Client{ + workingDir: tmp, + } + + got, err := c.RemoveFile("test", tt.filePath) + tt.wantErr(t, err) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/sonar-project.properties b/sonar-project.properties index 6d76668..b2d04cd 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -3,4 +3,4 @@ sonar.projectName=gerrit-operator sonar.projectVersion=1.0 sonar.go.coverage.reportPaths=coverage.out sonar.test.inclusions=**/*_test.go -sonar.exclusions=**/deploy-templates/**,api/edp/v1alpha1/*,**/main.go,**/*.groovy,**/.github/**,**/mock_*.go,**/mocks/**,**/mock/**,**/*_types.go,**/factory.go,**/config/**,**/build/**,Dockerfile +sonar.exclusions=**/deploy-templates/**,**/*generated.*.go,**/main.go,**/*.groovy,**/.github/**,**/mock_*.go,**/mocks/**,**/mock/**,**/*_types.go,**/factory.go,**/config/**,**/build/**,Dockerfile