Skip to content

Execute git clean partially when drift detection for every app is done #5312

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 7, 2024
Merged

Execute git clean partially when drift detection for every app is done #5312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
122119f
Execute git clean partially when drift detection for every app is done
ffjlabo Nov 6, 2024
ab2b33c
Rename to CleanPath
ffjlabo Nov 7, 2024
f1ef1a9
Add test for the outside dir pattern
ffjlabo Nov 7, 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
17 changes: 17 additions & 0 deletions pkg/app/piped/driftdetector/kubernetes/detector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,23 @@
if err := d.checkApplication(ctx, app, gitRepo, headCommit); err != nil {
d.logger.Error(fmt.Sprintf("failed to check application: %s", app.Id), zap.Error(err))
}

// Reset the app dir to the head commit.
// Some tools may create temporary files locally to render manifests.
// The detector reuses the same located local repository and it causes unexpected behavior by reusing such temporary files.
// So regularly run git clean on the app dir after each detection.
d.logger.Info("cleaning partially cloned repository",
zap.String("repo-id", repoID),
zap.String("app-id", app.Id),
zap.String("app-path", app.GitPath.Path),
)
if err := gitRepo.CleanPath(ctx, app.GitPath.Path); err != nil {
d.logger.Error("failed to clean partially cloned repository",
zap.String("repo-id", repoID),
zap.String("app-id", app.Id),
zap.String("app-path", app.GitPath.Path),
zap.Error(err))
}

Check warning on line 188 in pkg/app/piped/driftdetector/kubernetes/detector.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/driftdetector/kubernetes/detector.go#L177-L188 

Added lines #L177 - L188 were not covered by tests
}
}
}
Expand Down
14 changes: 14 additions & 0 deletions pkg/git/gittest/git.mock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions pkg/git/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ type Repo interface {
Checkout(ctx context.Context, commitish string) error
CheckoutPullRequest(ctx context.Context, number int, branch string) error
Clean() error
CleanPath(ctx context.Context, relativePath string) error

Pull(ctx context.Context, branch string) error
MergeRemoteBranch(ctx context.Context, branch, commit, mergeCommitMessage string) error
Expand Down Expand Up @@ -259,6 +260,15 @@ func (r repo) Clean() error {
return os.RemoveAll(r.dir)
}

// CleanPath deletes data in the given relative path in the repo with git clean.
func (r repo) CleanPath(ctx context.Context, relativePath string) error {
out, err := r.runGitCommand(ctx, "clean", "-f", relativePath)
Copy link
Member

Choose a reason for hiding this comment

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

Q: Since this is a relative path, do we need to check whether the given path points to an under dir or outer dir? (If it contains ../ or /, can the git clean command handle it carefully?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@khanhtc1202
git clean command checks whether the path is under the repo or not.
So IMO, we don't need the check. WDYT?

~/oss/pipe-cd/pipecd [fujiwo]
% git clean -f ../tutorial                    
fatal: ../tutorial: '../tutorial' is outside repository at '$HOME/oss/pipe-cd/pipecd

Copy link
Member Author

@ffjlabo ffjlabo Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

📝 I tried git clean at repo2 with both relative and absolute path.

at ~/test/repo2 
% git clean -f ../repo1/                                                         
fatal: ../repo1/: '../repo1/' is outside repository at '/$HOME/test/repo2'

at ~/test/repo2 
% git clean -f /$HOME/test/repo1                                          
fatal: /$HOME/test/repo1: '/$HOME/test/repo1' is outside repository at '/$HOME/test/repo2'

% tree .
.
├── repo1
│   └── README.md
├── repo2
       └── README.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Also added the test case for them f1ef1a9

Copy link
Member

Choose a reason for hiding this comment

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

That's nice 👍

if err != nil {
return formatCommandError(err, out)
}
return nil
}

func (r *repo) checkoutNewBranch(ctx context.Context, branch string) error {
out, err := r.runGitCommand(ctx, "checkout", "-b", branch)
if err != nil {
Expand Down
65 changes: 65 additions & 0 deletions pkg/git/repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,68 @@ func TestGetCommitForRev(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, commits[0].Hash, commit.Hash)
}

func TestCleanPath(t *testing.T) {
faker, err := newFaker()
require.NoError(t, err)
defer faker.clean()

var (
org = "test-repo-org"
repoName = "repo-clean-path"
ctx = context.Background()
)

err = faker.makeRepo(org, repoName)
require.NoError(t, err)
r := &repo{
dir: faker.repoDir(org, repoName),
gitPath: faker.gitPath,
}

// create two directories and a file in each
// repo-clean-path/part1/new-file.txt
// repo-clean-path/part2/new-file.txt
dirs := []string{"part1", "part2"}
for _, dir := range dirs {
partDir := filepath.Join(r.dir, dir)
err = os.MkdirAll(partDir, os.ModePerm)
require.NoError(t, err)

path := filepath.Join(partDir, "new-file.txt")
err = os.WriteFile(path, []byte("content"), os.ModePerm)
require.NoError(t, err)
}

// create other dir outside the repo
// repo-clean-path/outside-dir/new-file.txt
outsideDir := filepath.Join(r.dir, "..", "outside-dir")
require.NoError(t, err)

err = os.MkdirAll(outsideDir, os.ModePerm)
require.NoError(t, err)

path := filepath.Join(outsideDir, "new-file.txt")
err = os.WriteFile(path, []byte("content"), os.ModePerm)
require.NoError(t, err)

// clean the repo-dir/part1
err = r.CleanPath(ctx, "part1")
require.NoError(t, err)

// check the repo-dir/part1 is removed
_, err = os.Stat(filepath.Join(r.dir, "part1"))
assert.True(t, os.IsNotExist(err))

// check the repo-dir/part2 is still there
_, err = os.Stat(filepath.Join(r.dir, "part2"))
assert.NoError(t, err)

// check the outside dir can't be cleaned with relative path
err = r.CleanPath(ctx, "../outside-dir")
require.Error(t, err)

// check the outside dir can't be cleaned with relative path
err = r.CleanPath(ctx, outsideDir)
require.Error(t, err)
}
Loading