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

chore: code-refactoring #85

wants to merge 24 commits into from

Conversation

ShashwatDadhich
Copy link
Contributor

@ShashwatDadhich ShashwatDadhich commented Feb 1, 2024

Refactoring done to make structs and interfaces extended

@ShashwatDadhich ShashwatDadhich changed the title Git sensor sync chore: code-refactoring Feb 1, 2024
Comment on lines 379 to 397
func (impl *RepositoryManagerImpl) OpenNewRepo(gitCtx GitContext, location string, url string) (*GitRepository, error) {

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

Choose a reason for hiding this comment

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

fix

Comment on lines 137 to 156

func (impl *GoGitSDKManagerImpl) 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 r, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this here?

Comment on lines 195 to 213
func (impl *GitCliManagerImpl) 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 r, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed?

Comment on lines 78 to 87
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

@@ -148,16 +133,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

}

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

Comment on lines 11 to 14
//type GitCliManager interface {
// //GitManager
//}

Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Comment on lines 12 to 14
// type GoGitSDKManager interface {
// GitManager
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

type GoGitSDKManagerImpl struct {
*GitManagerBaseImpl
GitManagerBase
Copy link
Contributor

Choose a reason for hiding this comment

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

interface here but impl in cliManager. why?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we make interfaces in both places?

Copy link
Contributor Author

@ShashwatDadhich ShashwatDadhich Feb 2, 2024

Choose a reason for hiding this comment

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

Yes sure....Used interface in cliManager as well


updated, repo, err := impl.FetchAndUpdateMaterial(gitCtx, material, location)
if err != nil {
impl.logger.Errorw("error in fetching material details ", "repo", material.Url, "err", err)
// there might be the case if ssh private key gets flush from disk, so creating and single retrying in this case
// there might be the case if ssh priclvate key gets flush from disk, so creating and single retrying in this case
Copy link
Contributor

Choose a reason for hiding this comment

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

correct pls

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants