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

ffjlabo · 2024-11-06T09:59:17Z

What this PR does:

Fixed to execute git clean -f for every app dir after doing k8s drift detection.

Why we need it:

During drift detection, a Git repository is cloned and reused, and a common manifest cache is utilized for various operations like drift detection, plan preview, plan, and deploy.
If an incorrect manifest is cached, it can affect other processes. When running kustomize build --enable-helm, a charts directory is created, and the Helm chart is downloaded locally.
However, before kustomize v5.3.0, this directory is not updated if the Helm chart version in kustomization.yaml changes, causing drift detection to load outdated manifests.
These manifests are cached based on commit hash, leading to incorrect manifests being applied during Plan or Deploy if the same commit hash is used.

For the context, see #5124 (comment)

Which issue(s) this PR fixes:

Fixes #5124

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change: No
How to migrate (if breaking change):

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

codecov · 2024-11-06T10:04:35Z

Codecov Report

Attention: Patch coverage is 23.07692% with 20 lines in your changes missing coverage. Please review.

Project coverage is 25.26%. Comparing base (6b43655) to head (f1ef1a9).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/app/piped/driftdetector/kubernetes/detector.go	0.00%	12 Missing ⚠️
pkg/git/gittest/git.mock.go	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5312      +/-   ##
==========================================
- Coverage   25.27%   25.26%   -0.01%     
==========================================
  Files         444      444              
  Lines       47516    47554      +38     
==========================================
+ Hits        12010    12016       +6     
- Misses      34563    34596      +33     
+ Partials      943      942       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Warashi

LGTM

khanhtc1202 · 2024-11-07T01:28:14Z

pkg/git/repo.go

@@ -259,6 +260,15 @@ func (r repo) Clean() error {
 	return os.RemoveAll(r.dir)
 }

+// CleanPartially deletes data in the given relative path in the repo with git clean.
+func (r repo) CleanPartially(ctx context.Context, relativePath string) error {


Tbh I think partially is not suitable for the function name; how about renaming it CleanPath?

@khanhtc1202
Thank you, got it. Fixed in ab2b33c

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

khanhtc1202 · 2024-11-07T01:39:11Z

pkg/git/repo.go

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


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?)

@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

📝 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

Also added the test case for them f1ef1a9

That's nice 👍

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

khanhtc1202

LGTM 👍

ffjlabo · 2024-11-07T03:31:29Z

@Warashi Sorry, plz re-approve 🙏

Warashi

LGTM

#5312) * Execute git clean partially when drift detection for every app is done Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Rename to CleanPath Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add test for the outside dir pattern Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Add event context (#5295) * Add contexts to the RegisterEventRequest Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add contexts to model.Event Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Store event context in Control Plane Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add trailers when commiting on event watcher Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Fix for failed build Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Add docs for pipectl event register --contexts on the event watcher usage page (#5299) * Add docs for pipectl event register --contexts on the event watcher usage page Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Fix docs Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Fix command Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Clone manifests not to modify original manifests (#5306) Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Lambda: clone manifests not to modify original manifests (#5308) Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Skip commit and push when no replacement happens in EventWatcher (#5310) * fix: skip push if no diff exist Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * modify message Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * refactor: use noChange variable Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> --------- Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Execute git clean partially when drift detection for every app is done (#5312) * Execute git clean partially when drift detection for every app is done Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Rename to CleanPath Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add test for the outside dir pattern Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Update RELEASE and docs to v0.49.3 (#5315) Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Co-authored-by: Yoshiki Fujikane <40124947+ffjlabo@users.noreply.github.com> Co-authored-by: Tetsuya Kikuchi <97105818+t-kikuc@users.noreply.github.com>

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

122119f

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo requested review from nghialv, khanhtc1202, kentakozuka, t-kikuc and Warashi as code owners November 6, 2024 09:59

github-actions bot added the area/go label Nov 6, 2024

Warashi previously approved these changes Nov 7, 2024

View reviewed changes

khanhtc1202 reviewed Nov 7, 2024

View reviewed changes

Rename to CleanPath

ab2b33c

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo dismissed Warashi’s stale review via ab2b33c November 7, 2024 01:35

ffjlabo requested a review from khanhtc1202 November 7, 2024 01:36

khanhtc1202 reviewed Nov 7, 2024

View reviewed changes

Add test for the outside dir pattern

f1ef1a9

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo requested a review from khanhtc1202 November 7, 2024 02:52

khanhtc1202 approved these changes Nov 7, 2024

View reviewed changes

ffjlabo requested a review from Warashi November 7, 2024 03:31

Warashi approved these changes Nov 7, 2024

View reviewed changes

ffjlabo merged commit 7475ea8 into master Nov 7, 2024
16 of 18 checks passed

ffjlabo deleted the git-clean-when-drift-detection-is-done branch November 7, 2024 03:47

ffjlabo added cherry-pick v0.49.3 labels Nov 7, 2024

github-actions bot mentioned this pull request Nov 7, 2024

Update RELEASE and docs to v0.49.3 #5315

Merged

github-actions bot mentioned this pull request Nov 7, 2024

Cherry-pick #5295 #5299 #5306 #5308 #5310 #5312 #5315 #5316

Merged

peaceiris mentioned this pull request Nov 14, 2024

error: "failed to check application" and "failed to clean partially cloned repository" #5338

Open

github-actions bot mentioned this pull request Nov 20, 2024

Update RELEASE to v0.49.4 #5351

Merged

github-actions bot mentioned this pull request Dec 3, 2024

Make release version=v0.50.0 #5382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ffjlabo commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

Warashi left a comment

khanhtc1202 Nov 7, 2024

ffjlabo Nov 7, 2024

khanhtc1202 Nov 7, 2024

ffjlabo Nov 7, 2024

ffjlabo Nov 7, 2024 •

edited

Loading

ffjlabo Nov 7, 2024

khanhtc1202 Nov 7, 2024

khanhtc1202 left a comment

ffjlabo commented Nov 7, 2024

Warashi left a comment

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

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

Conversation

ffjlabo commented Nov 6, 2024 • edited Loading

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

Warashi left a comment

Choose a reason for hiding this comment

khanhtc1202 Nov 7, 2024

Choose a reason for hiding this comment

ffjlabo Nov 7, 2024

Choose a reason for hiding this comment

khanhtc1202 Nov 7, 2024

Choose a reason for hiding this comment

ffjlabo Nov 7, 2024

Choose a reason for hiding this comment

ffjlabo Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

ffjlabo Nov 7, 2024

Choose a reason for hiding this comment

khanhtc1202 Nov 7, 2024

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

ffjlabo commented Nov 7, 2024

Warashi left a comment

Choose a reason for hiding this comment

ffjlabo commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

ffjlabo Nov 7, 2024 •

edited

Loading