Add time-based cleanup for Maven snapshot versions #33420

dianaStr7 · 2025-01-27T21:06:39Z

Pull Request: Implement Cleanup Function for Maven Snapshot Versions

Overview

This pull request introduces a cleanup function for Maven snapshot versions in Gitea, enabling more efficient management of package storage. The new feature allows users to specify how many of the most recent Maven snapshot builds to retain, optimizing storage by automatically removing older files.

Features

New Configurable Variable: RETAIN_MAVEN_SNAPSHOT_BUILDS allows setting the number of Maven snapshot builds to keep.
- Default is -1, which keeps all builds.
Automated Cleanup: Tied to the cleanup expired data process, this function targets files within Maven snapshots, not the versions themselves, ensuring that only essential files are kept.

Implementation

The feature extends the existing cleanup job to include files from Maven snapshot versions based on the specified retention policy set in app.ini. It checks against the highest build number from the Maven metadata file to determine which files to retain.

Impact

This enhancement helps manage disk space more effectively by providing control over how many builds of Maven snapshots are retained, potentially reducing storage requirements for projects using Maven within Gitea.

dianaStr7 · 2025-01-27T21:13:32Z

Considered Alternative Approach

I considered another way to handle Maven snapshot cleanup that uses the properties field for package files to store build numbers right when files are uploaded. This would make database searches simpler.

This method would need a database migration to work, so it's a bit more complicated. I have the code ready for this alternative if we think it's worth the extra steps later on.

delvh · 2025-01-31T17:29:34Z

Note to any reviewer: Review this PR carefully.
It seems like an LLM at the very least helped with creating this PR.
That in and of itself is nothing bad, as long as the output is indeed correct.
Given my experience with ChatGPT, I find it rather likely that it missed some edge cases though.

dianaStr7 · 2025-02-20T09:47:05Z

A remark: Yes, it's true that I used ChatGPT for help since I'm not an experienced Go developer, but I handled the core logic and testing myself. I'd appreciate any comments or critiques to help us improve this, as we really need the feature :)

Note to any reviewer: Review this PR carefully. It seems like an LLM at the very least helped with creating this PR. That in and of itself is nothing bad, as long as the output is indeed correct. Given my experience with ChatGPT, I find it rather likely that it missed some edge cases though.

delvh · 2025-01-31T17:20:05Z

modules/setting/packages.go

@@ -101,7 +103,7 @@ func loadPackagesFrom(rootCfg ConfigProvider) (err error) {
 	Packages.LimitSizeRubyGems = mustBytes(sec, "LIMIT_SIZE_RUBYGEMS")
 	Packages.LimitSizeSwift = mustBytes(sec, "LIMIT_SIZE_SWIFT")
 	Packages.LimitSizeVagrant = mustBytes(sec, "LIMIT_SIZE_VAGRANT")
-	Packages.DefaultRPMSignEnabled = sec.Key("DEFAULT_RPM_SIGN_ENABLED").MustBool(false)


This line seems to have been deleted accidentally

delvh · 2025-01-31T17:45:11Z

modules/packages/maven/metadata.go

+	var metadata MavenMetadata
+
+	dec := xml.NewDecoder(r)
+	dec.CharsetReader = charset.NewReaderLabel // Assuming charset.NewReaderLabel is a function you've set up to handle character encoding.


Erm...
I mean, it should probably work if I'm reading the documentation correctly, but the comment seems really out of place

Suggested change

dec.CharsetReader = charset.NewReaderLabel // Assuming charset.NewReaderLabel is a function you've set up to handle character encoding.

dec.CharsetReader = charset.NewReaderLabel

delvh · 2025-01-31T17:50:45Z

modules/packages/maven/metadata.go

@@ -109,3 +131,20 @@ func ParsePackageMetaData(r io.Reader) (*Metadata, error) {
 		Dependencies: dependencies,
 	}, nil
 }
+
+// ParseMavenMetadata parses the Maven metadata XML to extract the build number.
+func ParseMavenMetaData(r io.Reader) (string, error) {


I am thoroughly confused by this method signature and docs: Nothing indicates to me that this method parses the entire XML, and throws the result except for the build number away.

delvh · 2025-02-20T14:07:58Z

models/packages/package_file.go

+		}
+	}
+
+	log.Info("Filtered %d files out of %d total files for version ID %d with maxBuildNumber %d", len(filteredFiles), len(files), versionID, maxBuildNumber)


Hmm… I do not like seeing log statements inside models/.
That should be the job of the services/ layer.
Thus, what about adding an extra return value (skippedFiles) that contains all skipped files?

delvh · 2025-02-20T14:09:46Z

models/packages/package_file.go

+		}
+	}
+
+	log.Info("Filtered %d files out of %d total files for version ID %d with maxBuildNumber %d", len(filteredFiles), len(files), versionID, maxBuildNumber)


Info logs are rare within Gitea.
At most, gather statistics inside a success message after successfully running the cron job, or downgrade it to debug.

delvh · 2025-02-20T22:03:55Z

services/packages/maven/cleanup.go

+}
+
+func isSnapshotVersion(version string) bool {
+	return strings.Contains(version, "-SNAPSHOT")


Isn't it even HasSuffix?

delvh · 2025-02-20T22:06:20Z

models/packages/package_file.go

@@ -226,6 +229,62 @@ func HasFiles(ctx context.Context, opts *PackageFileSearchOptions) (bool, error)
 	return db.Exist[PackageFile](ctx, opts.toConds())
 }

+// GetFilesByBuildNumber retrieves all files for a package version with build numbers <= maxBuildNumber.
+func GetFilesByBuildNumber(ctx context.Context, versionID int64, maxBuildNumber int) ([]*PackageFile, error) {


Isn't that rather GetFilesBelowBuildNumber instead?

delvh · 2025-02-20T22:07:40Z

models/packages/package_file.go

+
+	buildNumber, err := strconv.Atoi(buildNumberStr)
+	if err != nil {
+		return 0, fmt.Errorf("failed to convert build number to integer: '%s'", buildNumberStr)


Suggested change

return 0, fmt.Errorf("failed to convert build number to integer: '%s'", buildNumberStr)

return 0, fmt.Errorf("failed to convert maven package build number to integer: '%s'", buildNumberStr)

delvh · 2025-02-20T22:10:00Z

services/packages/maven/cleanup.go

+
+	thresholdBuildNumber := maxBuildNumber - retainBuilds
+	if thresholdBuildNumber <= 0 {
+		log.Info("No files to clean up, as the threshold build number is less than or equal to zero for versionID %d", versionID)


Regarding the error messages and logs in general, I recommend explicitly stating during what sort of action they occurred.
When you actually see these logs, it will be hard to guess that they belong to the Maven snapshot cleanup otherwise, especially when a request happens in the meantime.

delvh · 2025-02-20T22:11:32Z

services/packages/maven/cleanup.go

+	return nil
+}
+
+func extractMaxBuildNumberFromMetadata(ctx context.Context, metadataFile *packages.PackageFile) (int, error) {


Didn't you define this method in models already?

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 27, 2025

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 27, 2025

github-actions bot added modifies/go Pull requests that update Go code docs-update-needed The document needs to be updated synchronously labels Jan 27, 2025

dianaStr7 mentioned this pull request Jan 28, 2025

Gitea Maven Package Management provides no cleanup rules for limiting the number of SNAPSHOTs kept for versions #32107

Open

delvh changed the title ~~Added cleanup method for files in Maven snapshot versions~~ Add time-based cleanup for Maven snapshot versions Jan 31, 2025

delvh requested a review from KN4CK3R January 31, 2025 17:30

Added cleanup method for files in Maven snapshot versions

f7960a4

dianaStr7 force-pushed the main branch from e8e4bbc to f7960a4 Compare February 20, 2025 09:43

delvh requested changes Feb 20, 2025

View reviewed changes

GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add time-based cleanup for Maven snapshot versions #33420

Add time-based cleanup for Maven snapshot versions #33420

dianaStr7 commented Jan 27, 2025

dianaStr7 commented Jan 27, 2025

delvh commented Jan 31, 2025

dianaStr7 commented Feb 20, 2025

delvh Jan 31, 2025

delvh Jan 31, 2025

delvh Jan 31, 2025

delvh Feb 20, 2025

delvh Feb 20, 2025

delvh Feb 20, 2025

delvh Feb 20, 2025

delvh Feb 20, 2025

delvh Feb 20, 2025

delvh Feb 20, 2025

	dec.CharsetReader = charset.NewReaderLabel // Assuming charset.NewReaderLabel is a function you've set up to handle character encoding.
	dec.CharsetReader = charset.NewReaderLabel

	return 0, fmt.Errorf("failed to convert build number to integer: '%s'", buildNumberStr)
	return 0, fmt.Errorf("failed to convert maven package build number to integer: '%s'", buildNumberStr)

Add time-based cleanup for Maven snapshot versions #33420

Are you sure you want to change the base?

Add time-based cleanup for Maven snapshot versions #33420

Conversation

dianaStr7 commented Jan 27, 2025

Pull Request: Implement Cleanup Function for Maven Snapshot Versions

Overview

Features

Implementation

Impact

dianaStr7 commented Jan 27, 2025

Considered Alternative Approach

delvh commented Jan 31, 2025

dianaStr7 commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment