Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: code-refactoring #85

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
a927028
code refactoring
ShashwatDadhich Jan 31, 2024
870512a
internal name changed to internals
ShashwatDadhich Jan 31, 2024
afea9c7
code made common
ShashwatDadhich Jan 31, 2024
12688e0
code refactoring in GitBaseManager.go
ShashwatDadhich Jan 31, 2024
ab32ad7
code refactoring in GitBaseManager.go
ShashwatDadhich Jan 31, 2024
36e964e
code refactoring in GitBaseManager.go
ShashwatDadhich Jan 31, 2024
135bca7
code refactoring
ShashwatDadhich Jan 31, 2024
22a0c6a
code refactoring
ShashwatDadhich Feb 1, 2024
6959802
code refactoring
ShashwatDadhich Feb 1, 2024
4590a56
code refactoring
ShashwatDadhich Feb 1, 2024
87c7fd6
code refactoring
ShashwatDadhich Feb 1, 2024
1e4829a
code refactoring as suggested in code review
ShashwatDadhich Feb 1, 2024
2bbe296
commented code removed
ShashwatDadhich Feb 1, 2024
a02805a
commented code retrieved
ShashwatDadhich Feb 1, 2024
e9b3856
GitManagerImpl retrieved
ShashwatDadhich Feb 1, 2024
2a6a05a
GitManagerImpl - wire file corrected
ShashwatDadhich Feb 1, 2024
b23e4f3
code review comments incorporated
ShashwatDadhich Feb 2, 2024
32b9bd3
code review comments incorporated
ShashwatDadhich Feb 2, 2024
9543583
app directory created
ShashwatDadhich Feb 2, 2024
49f1498
code review comments incorporated
ShashwatDadhich Feb 2, 2024
8d9f6d5
code review comments incorporated
ShashwatDadhich Feb 2, 2024
8028b7e
code review comments incorporated
ShashwatDadhich Feb 2, 2024
bccadd8
wireSet generated
ShashwatDadhich Feb 2, 2024
7e70ef9
wireSet and test file changed
ShashwatDadhich Feb 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion App.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
pubsub "github.com/devtron-labs/common-lib/pubsub-lib"
"github.com/devtron-labs/git-sensor/api"
"github.com/devtron-labs/git-sensor/bean"
"github.com/devtron-labs/git-sensor/internal/middleware"
"github.com/devtron-labs/git-sensor/internals/middleware"
"github.com/devtron-labs/git-sensor/pkg/git"
pb "github.com/devtron-labs/protos/gitSensor"
"github.com/go-pg/pg"
Expand Down
3 changes: 2 additions & 1 deletion api/GrpcHandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package api

import (
"context"
"github.com/devtron-labs/git-sensor/internal/sql"
"github.com/devtron-labs/git-sensor/internals/sql"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal directory - is a special name that do not allow others to import any package under this directory. So after renaming it to internals - anyone can import this packages. I am almost sure that this is not a main goal of this PR.

"github.com/devtron-labs/git-sensor/pkg"
"github.com/devtron-labs/git-sensor/pkg/git"
pb "github.com/devtron-labs/protos/gitSensor"
Expand Down Expand Up @@ -355,6 +355,7 @@ func (impl *GrpcHandlerImpl) GetCommitMetadataForPipelineMaterial(ctx context.Co

return nil, err
}

if res == nil {
res1 := &pb.GitCommit{}
return res1, nil
Expand Down
2 changes: 1 addition & 1 deletion api/GrpcHandler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package api
import (
"context"
"encoding/json"
"github.com/devtron-labs/git-sensor/internal/sql"
"github.com/devtron-labs/git-sensor/internals/sql"
"github.com/devtron-labs/git-sensor/pkg"
"github.com/devtron-labs/git-sensor/pkg/git"
"github.com/devtron-labs/git-sensor/pkg/mocks"
Expand Down
2 changes: 1 addition & 1 deletion api/RestHandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package api

import (
"encoding/json"
"github.com/devtron-labs/git-sensor/internal/sql"
"github.com/devtron-labs/git-sensor/internals/sql"
"github.com/devtron-labs/git-sensor/pkg"
"github.com/devtron-labs/git-sensor/pkg/git"
"github.com/gorilla/mux"
Expand Down
7 changes: 3 additions & 4 deletions internal/Configuration.go → internals/Configuration.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
package internal
package internals

import (
"github.com/caarlos0/env"
)
import "github.com/caarlos0/env"

type Configuration struct {
CommitStatsTimeoutInSec int `env:"COMMIT_STATS_TIMEOUT_IN_SEC" envDefault:"2"`
EnableFileStats bool `env:"ENABLE_FILE_STATS" envDefault:"false"`
GitHistoryCount int `env:"GIT_HISTORY_COUNT" envDefault:"15"`
CloningMode string `env:"CLONING_MODE" envDefault:"FULL"`
MinLimit int `env:"MIN_LIMIT_FOR_PVC" envDefault:"1"` // in MB
UseGitCli bool `env:"USE_GIT_CLI" envDefault:"false"`
AnalyticsDebug bool `env:"ANALYTICS_DEBUG" envDefault:"false"`
Expand Down
2 changes: 1 addition & 1 deletion internal/Lockutil.go → internals/Lockutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

package internal
package internals

import (
"go.uber.org/zap"
Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package util

import (
"github.com/devtron-labs/git-sensor/internal/sql"
"github.com/devtron-labs/git-sensor/internals/sql"
"strings"
)

Expand Down
44 changes: 29 additions & 15 deletions pkg/RepoManages.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/devtron-labs/git-sensor/internal"
"github.com/devtron-labs/git-sensor/internal/sql"
"github.com/devtron-labs/git-sensor/internal/util"
"github.com/devtron-labs/git-sensor/internals"
"github.com/devtron-labs/git-sensor/internals/sql"
"github.com/devtron-labs/git-sensor/internals/util"
"github.com/devtron-labs/git-sensor/pkg/git"
_ "github.com/robfig/cron/v3"
"go.uber.org/zap"
Expand Down Expand Up @@ -58,15 +58,15 @@ type RepoManagerImpl struct {
repositoryManagerAnalytics git.RepositoryManagerAnalytics
gitProviderRepository sql.GitProviderRepository
ciPipelineMaterialRepository sql.CiPipelineMaterialRepository
locker *internal.RepositoryLocker
locker *internals.RepositoryLocker
gitWatcher git.GitWatcher
webhookEventRepository sql.WebhookEventRepository
webhookEventParsedDataRepository sql.WebhookEventParsedDataRepository
webhookEventDataMappingRepository sql.WebhookEventDataMappingRepository
webhookEventDataMappingFilterResultRepository sql.WebhookEventDataMappingFilterResultRepository
webhookEventBeanConverter git.WebhookEventBeanConverter
configuration *internal.Configuration
gitManager git.GitManagerImpl
configuration *internals.Configuration
gitManager git.GitManager
}

func NewRepoManagerImpl(
Expand All @@ -76,14 +76,14 @@ func NewRepoManagerImpl(
repositoryManagerAnalytics git.RepositoryManagerAnalytics,
gitProviderRepository sql.GitProviderRepository,
ciPipelineMaterialRepository sql.CiPipelineMaterialRepository,
locker *internal.RepositoryLocker,
locker *internals.RepositoryLocker,
gitWatcher git.GitWatcher, webhookEventRepository sql.WebhookEventRepository,
webhookEventParsedDataRepository sql.WebhookEventParsedDataRepository,
webhookEventDataMappingRepository sql.WebhookEventDataMappingRepository,
webhookEventDataMappingFilterResultRepository sql.WebhookEventDataMappingFilterResultRepository,
webhookEventBeanConverter git.WebhookEventBeanConverter,
configuration *internal.Configuration,
gitManager git.GitManagerImpl,
configuration *internals.Configuration,
gitManager git.GitManager,
) *RepoManagerImpl {
return &RepoManagerImpl{
logger: logger,
Expand Down Expand Up @@ -195,6 +195,9 @@ func (impl RepoManagerImpl) updatePipelineMaterialCommit(gitCtx git.GitContext,
continue
}

gitCtx = gitCtx.WithCredentials(material.GitProvider.UserName, material.GitProvider.Password).
WithCloningMode(impl.configuration.CloningMode)

commits, err := impl.repositoryManager.ChangesSince(gitCtx, material.CheckoutLocation, pipelineMaterial.Value, "", "", impl.configuration.GitHistoryCount)
//commits, err := impl.FetchChanges(pipelineMaterial.Id, "", "", 0)
if err == nil {
Expand Down Expand Up @@ -345,15 +348,18 @@ func (impl RepoManagerImpl) checkoutMaterial(gitCtx git.GitContext, material *sq
if err != nil {
return material, nil
}
checkoutPath, err := git.GetLocationForMaterial(material)

gitCtx = gitCtx.WithCredentials(userName, password).
WithCloningMode(impl.configuration.CloningMode)

checkoutPath, checkoutLocationForFetching, err := impl.repositoryManager.GetCheckoutPathAndLocation(gitCtx, material, gitProvider.Url)
if err != nil {
return material, err
}
gitCtx = gitCtx.WithCredentials(userName, password)

err = impl.repositoryManager.Add(gitCtx, material.GitProviderId, checkoutPath, material.Url, gitProvider.AuthMode, gitProvider.SshPrivateKey)
if err == nil {
material.CheckoutLocation = checkoutPath
material.CheckoutLocation = checkoutLocationForFetching
material.CheckoutStatus = true
} else {
material.CheckoutStatus = false
Expand Down Expand Up @@ -627,7 +633,10 @@ func (impl RepoManagerImpl) GetLatestCommitForBranch(gitCtx git.GitContext, pipe
}()

userName, password, err := git.GetUserNamePassword(gitMaterial.GitProvider)
gitCtx = gitCtx.WithCredentials(userName, password)

gitCtx = gitCtx.WithCredentials(userName, password).
WithCloningMode(impl.configuration.CloningMode)

updated, repo, err := impl.repositoryManager.Fetch(gitCtx, gitMaterial.Url, gitMaterial.CheckoutLocation)
if !updated {
impl.logger.Warn("repository is up to date")
Expand Down Expand Up @@ -659,7 +668,7 @@ func (impl RepoManagerImpl) GetLatestCommitForBranch(gitCtx git.GitContext, pipe
return nil, err
}

commits, err := impl.repositoryManager.ChangesSinceByRepository(gitCtx, repo, branchName, "", "", 1)
commits, err := impl.repositoryManager.ChangesSinceByRepository(gitCtx, repo, branchName, "", "", 1, gitMaterial.CheckoutLocation)

if commits == nil {
return nil, err
Expand Down Expand Up @@ -694,6 +703,8 @@ func (impl RepoManagerImpl) GetCommitMetadataForPipelineMaterial(gitCtx git.GitC
return nil, err
}

gitCtx = gitCtx.WithCredentials(gitMaterial.GitProvider.UserName, gitMaterial.GitProvider.Password).
WithCloningMode(impl.configuration.CloningMode)
// validate checkout status of gitMaterial
if !gitMaterial.CheckoutStatus {
impl.logger.Errorw("checkout not success", "gitMaterialId", gitMaterialId)
Expand All @@ -707,7 +718,6 @@ func (impl RepoManagerImpl) GetCommitMetadataForPipelineMaterial(gitCtx git.GitC
repoLock.Mutex.Unlock()
impl.locker.ReturnLocker(gitMaterial.Id)
}()

commits, err := impl.repositoryManager.ChangesSince(gitCtx, gitMaterial.CheckoutLocation, branchName, "", gitHash, 1)
if err != nil {
impl.logger.Errorw("error while fetching commit info", "pipelineMaterialId", pipelineMaterialId, "gitHash", gitHash, "err", err)
Expand Down Expand Up @@ -747,6 +757,10 @@ func (impl RepoManagerImpl) GetReleaseChanges(gitCtx git.GitContext, request *Re
repoLock.Mutex.Unlock()
impl.locker.ReturnLocker(gitMaterial.Id)
}()

gitCtx = gitCtx.WithCredentials(gitMaterial.GitProvider.UserName, gitMaterial.GitProvider.Password).
WithCloningMode(impl.configuration.CloningMode)

gitChanges, err := impl.repositoryManagerAnalytics.ChangesSinceByRepositoryForAnalytics(gitCtx, gitMaterial.CheckoutLocation, request.OldCommit, request.NewCommit)
if err != nil {
impl.logger.Errorw("error in computing changes", "req", request, "err", err)
Expand Down
2 changes: 1 addition & 1 deletion pkg/git/Bean.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
package git

import (
"github.com/devtron-labs/git-sensor/internal/sql"
"github.com/devtron-labs/git-sensor/internals/sql"
"gopkg.in/src-d/go-git.v4"
"gopkg.in/src-d/go-git.v4/plumbing/object"
"io"
Expand Down
63 changes: 25 additions & 38 deletions pkg/git/GitBaseManager.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/devtron-labs/git-sensor/internal"
"github.com/devtron-labs/git-sensor/internal/sql"
"github.com/devtron-labs/git-sensor/internals"
"github.com/devtron-labs/git-sensor/internals/sql"
"github.com/devtron-labs/git-sensor/util"
"go.uber.org/zap"
"os"
Expand All @@ -29,8 +29,6 @@ type GitManager interface {
OpenRepoPlain(checkoutPath string) (*GitRepository, error)
// Init initializes a git repo
Init(gitCtx GitContext, rootDir string, remoteUrl string, isBare bool) error
// FetchDiffStatBetweenCommits returns the file stats reponse on executing git action
FetchDiffStatBetweenCommits(gitCtx GitContext, oldHash string, newHash string, rootDir string) (response, errMsg string, err error)
}

// GitManagerBase Base methods which will be available to all implementation of the parent interface
Expand All @@ -43,16 +41,21 @@ type GitManagerBase interface {
Checkout(gitCtx GitContext, rootDir, branch string) (response, errMsg string, err error)
// ConfigureSshCommand configures ssh in git repo
ConfigureSshCommand(gitCtx GitContext, rootDir string, sshPrivateKeyPath string) (response, errMsg string, err error)
// FetchDiffStatBetweenCommits returns the file stats reponse on executing git action
FetchDiffStatBetweenCommits(gitCtx GitContext, oldHash string, newHash string, rootDir string) (response, errMsg string, err error)
// LogMergeBase get the commit diff between using a merge base strategy
LogMergeBase(gitCtx GitContext, rootDir, from string, to string) ([]*Commit, error)
CreateCmdWithContext(ctx GitContext, name string, arg ...string) (*exec.Cmd, context.CancelFunc)
RunCommandWithCred(cmd *exec.Cmd, userName, password string) (response, errMsg string, err error)
RunCommand(cmd *exec.Cmd) (response, errMsg string, err error)
}
type GitManagerBaseImpl struct {
logger *zap.SugaredLogger
conf *internal.Configuration
conf *internals.Configuration
commandTimeoutMap map[string]int
}

func NewGitManagerBaseImpl(logger *zap.SugaredLogger, config *internal.Configuration) *GitManagerBaseImpl {
func NewGitManagerBaseImpl(logger *zap.SugaredLogger, config *internals.Configuration) *GitManagerBaseImpl {

commandTimeoutMap, err := parseCmdTimeoutJson(config)
if err != nil {
Expand All @@ -62,7 +65,7 @@ func NewGitManagerBaseImpl(logger *zap.SugaredLogger, config *internal.Configura
return &GitManagerBaseImpl{logger: logger, conf: config, commandTimeoutMap: commandTimeoutMap}
}

func parseCmdTimeoutJson(config *internal.Configuration) (map[string]int, error) {
func parseCmdTimeoutJson(config *internals.Configuration) (map[string]int, error) {
commandTimeoutMap := make(map[string]int)
var err error
if config.CliCmdTimeoutJson != "" {
Expand All @@ -75,41 +78,25 @@ type GitManagerImpl struct {
GitManager
}

func NewGitManagerImpl(configuration *internal.Configuration,
func NewGitManagerImpl(configuration *internals.Configuration,
cliGitManager GitCliManager,
goGitManager GoGitSDKManager) GitManagerImpl {
goGitManager GoGitSDKManager) *GitManagerImpl {

if configuration.UseGitCli {
return GitManagerImpl{cliGitManager}
}
return GitManagerImpl{goGitManager}
}

func (impl *GitManagerImpl) OpenNewRepo(gitCtx GitContext, location string, url string) (*GitRepository, error) {

r, err := impl.OpenRepoPlain(location)
if err != nil {
err = os.RemoveAll(location)
if err != nil {
return r, fmt.Errorf("error in cleaning checkout path: %s", err)
}
err = impl.Init(gitCtx, location, url, true)
if err != nil {
return r, fmt.Errorf("err in git init: %s", err)
}
r, err = impl.OpenRepoPlain(location)
if err != nil {
return r, fmt.Errorf("err in git init: %s", err)
return &GitManagerImpl{
cliGitManager,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to have a look

}
}
return r, nil
return &GitManagerImpl{
goGitManager,
}
}

func (impl *GitManagerBaseImpl) Fetch(gitCtx GitContext, rootDir string) (response, errMsg string, err error) {
impl.logger.Debugw("git fetch ", "location", rootDir)
cmd, cancel := impl.CreateCmdWithContext(gitCtx, "git", "-C", rootDir, "fetch", "origin", "--tags", "--force")
defer cancel()
output, errMsg, err := impl.runCommandWithCred(cmd, gitCtx.Username, gitCtx.Password)
output, errMsg, err := impl.RunCommandWithCred(cmd, gitCtx.Username, gitCtx.Password)
impl.logger.Debugw("fetch output", "root", rootDir, "opt", output, "errMsg", errMsg, "error", err)
return output, errMsg, err
}
Expand All @@ -118,7 +105,7 @@ func (impl *GitManagerBaseImpl) Checkout(gitCtx GitContext, rootDir, branch stri
impl.logger.Debugw("git checkout ", "location", rootDir)
cmd, cancel := impl.CreateCmdWithContext(gitCtx, "git", "-C", rootDir, "checkout", branch, "--force")
defer cancel()
output, errMsg, err := impl.runCommand(cmd)
output, errMsg, err := impl.RunCommand(cmd)
impl.logger.Debugw("checkout output", "root", rootDir, "opt", output, "errMsg", errMsg, "error", err)
return output, errMsg, err
}
Expand All @@ -135,7 +122,7 @@ func (impl *GitManagerBaseImpl) LogMergeBase(gitCtx GitContext, rootDir, from st
impl.logger.Debugw("git", cmdArgs)
cmd, cancel := impl.CreateCmdWithContext(gitCtx, "git", cmdArgs...)
defer cancel()
output, errMsg, err := impl.runCommand(cmd)
output, errMsg, err := impl.RunCommand(cmd)
impl.logger.Debugw("root", rootDir, "opt", output, "errMsg", errMsg, "error", err)
if err != nil {
return nil, err
Expand All @@ -148,16 +135,16 @@ func (impl *GitManagerBaseImpl) LogMergeBase(gitCtx GitContext, rootDir, from st
return commits, nil
}

func (impl *GitManagerBaseImpl) runCommandWithCred(cmd *exec.Cmd, userName, password string) (response, errMsg string, err error) {
func (impl *GitManagerBaseImpl) RunCommandWithCred(cmd *exec.Cmd, userName, password string) (response, errMsg string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why public

cmd.Env = append(os.Environ(),
fmt.Sprintf("GIT_ASKPASS=%s", GIT_ASK_PASS),
fmt.Sprintf("GIT_USERNAME=%s", userName),
fmt.Sprintf("GIT_PASSWORD=%s", password),
)
return impl.runCommand(cmd)
return impl.RunCommand(cmd)
}

func (impl *GitManagerBaseImpl) runCommand(cmd *exec.Cmd) (response, errMsg string, err error) {
func (impl *GitManagerBaseImpl) RunCommand(cmd *exec.Cmd) (response, errMsg string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why public

cmd.Env = append(cmd.Env, "HOME=/dev/null")
outBytes, err := cmd.CombinedOutput()
if err != nil {
Expand All @@ -183,7 +170,7 @@ func (impl *GitManagerBaseImpl) ConfigureSshCommand(gitCtx GitContext, rootDir s
coreSshCommand := fmt.Sprintf("ssh -i %s -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no", sshPrivateKeyPath)
cmd, cancel := impl.CreateCmdWithContext(gitCtx, "git", "-C", rootDir, "config", "core.sshCommand", coreSshCommand)
defer cancel()
output, errMsg, err := impl.runCommand(cmd)
output, errMsg, err := impl.RunCommand(cmd)
impl.logger.Debugw("configure ssh command output ", "root", rootDir, "opt", output, "errMsg", errMsg, "error", err)
return output, errMsg, err
}
Expand Down Expand Up @@ -280,7 +267,7 @@ func (impl *GitManagerBaseImpl) FetchDiffStatBetweenCommits(gitCtx GitContext, o
cmd, cancel := impl.CreateCmdWithContext(gitCtx, "git", "-C", rootDir, "diff", "--numstat", oldHash, newHash)
defer cancel()

output, errMsg, err := impl.runCommandWithCred(cmd, gitCtx.Username, gitCtx.Password)
output, errMsg, err := impl.RunCommandWithCred(cmd, gitCtx.Username, gitCtx.Password)
impl.logger.Debugw("root", rootDir, "opt", output, "errMsg", errMsg, "error", err)
return output, errMsg, err
}
Expand Down
Loading