fix: remove empty trust store directory after notation cert delete and notation cert cleanup-test

[ghost]

Resolves #1239

[codecov]

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.96%. Comparing base (4b873a7) to head (7ce9d79).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/notation/internal/truststore/truststore.go	70.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1243      +/-   ##
==========================================
+ Coverage   76.91%   76.96%   +0.05%     
==========================================
  Files          68       68              
  Lines        3825     3847      +22     
==========================================
+ Hits         2942     2961      +19     
- Misses        680      682       +2     
- Partials      203      204       +1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull Request Overview

This PR resolves issue #1239 by ensuring that empty trust store directories are removed after executing certificate deletion and cleanup commands. Key changes include adding tests for verifying directory emptiness, enhancing end-to-end tests to check for proper cleanup of trust store directories, and modifying the trust store deletion logic to remove the directory when it is empty.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
internal/osutil/file_test.go	Added tests for IsDirEmpty to verify directory emptiness and error handling.
test/e2e/suite/command/cert.go	Extended e2e tests for cert deletion and cleanup to include trust store checks.
internal/osutil/file.go	Added IsDirEmpty function with appropriate error handling.
cmd/notation/internal/truststore/truststore.go	Updated DeleteCert to remove an empty trust store directory after deletion.

Comments suppressed due to low confidence (3)

internal/osutil/file_test.go:278

• Consider checking the error return from IsDirEmpty first before asserting the emptiness of the directory to ensure any underlying failure is caught.

if !isEmpty {

test/e2e/suite/command/cert.go:46

• [nitpick] The repeated check for the trust store directory across multiple tests suggests a refactoring opportunity; consider extracting this assertion into a helper function to reduce duplication.

Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))

cmd/notation/internal/truststore/truststore.go:205

• [nitpick] Consider using a structured logging mechanism instead of fmt.Fprintf for warnings, which can improve consistency and testability in production environments.

fmt.Fprintf(os.Stderr, "Warning: failed to remove the empty trust store directory %s: %v\n", storePath, err)

[Copilot]

Pull Request Overview

This PR resolves #1239 by ensuring that the trust store directory is removed when empty after running "notation cert delete" and "notation cert cleanup-test".

• Added tests to verify proper deletion or preservation of the trust store directory under various conditions.
• Updated the DeleteCert function to attempt removal of the empty trust store directory and log warnings on errors.
• Enhanced error and permission handling in the osutil package tests.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
test/e2e/suite/command/cert.go	Added tests to confirm that empty trust store directories are removed where expected.
internal/osutil/file_test.go	Added tests for IsDirEmpty, including error and permission scenarios.
internal/osutil/file.go	Introduced IsDirEmpty to determine if a directory is empty.
cmd/notation/internal/truststore/truststore.go	Updated DeleteCert to remove the trust store directory if empty and log warnings.

Comments suppressed due to low confidence (2)

test/e2e/suite/command/cert.go:44

• [nitpick] Consider extracting the repeated trust store path computation into a helper function to improve maintainability and reduce duplication across the tests.

trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e")

internal/osutil/file_test.go:304

• [nitpick] Consider using error type assertions or dedicated error-checking utilities instead of substring matching to robustly verify permission errors.

expectedErrorMsg := "permission denied"

[Copilot]

Pull Request Overview

This PR fixes issue #1239 by ensuring that empty trust store directories are removed after certificate deletion and cleanup operations. Key changes include:

• Adding a new function IsDirEmpty and corresponding tests.
• Updating the trust store deletion logic in DeleteCert to remove the empty trust store directory.
• Enhancing e2e tests to verify the proper removal or retention of trust store directories.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/osutil/file_test.go	Added tests for IsDirEmpty, including scenarios with non-existent directories and permission errors.
internal/osutil/file.go	Introduced IsDirEmpty to check for empty directories.
cmd/notation/internal/truststore/truststore.go	Updated DeleteCert to remove an empty trust store directory post deletion of a certificate.
cmd/notation/cert/delete.go	Minor formatting and messaging changes aligning with the updated deletion behavior.
test/e2e/suite/command/cert.go	Added e2e tests to verify trust store directory existence following certificate operations.

Comments suppressed due to low confidence (2)

internal/osutil/file_test.go:287

• [nitpick] Consider renaming the variable 'nonExistDir' to 'nonExistentDir' for improved clarity.

nonExistDir := "invalid"

cmd/notation/internal/truststore/truststore.go:196

• When osutil.IsDirEmpty returns an error, the code logs a warning and assumes 'empty' is false. Consider propagating the error or explicitly handling this case to avoid potential silent failures.

if err != nil {

[shizhMSFT]

LGTM

[JeyJeyGao]

LGTM