fix: package cleanup rules are not applied when there are more than 200 packages (depends on MAX_RESPONSE_ITEMS) (#9219)
		
	Before it has only processed the newest 200 (or 50 for default `MAX_RESPONSE_ITEMS: 50`) versions. After it processes all versions. <!--start release-notes-assistant--> ## Release notes <!--URL:https://codeberg.org/forgejo/forgejo--> - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/9219): <!--number 9219 --><!--line 0 --><!--description cGFja2FnZSBjbGVhbnVwIHJ1bGVzIGFyZSBub3QgYXBwbGllZCB3aGVuIHRoZXJlIGFyZSBtb3JlIHRoYW4gMjAwIHBhY2thZ2VzIChkZXBlbmRzIG9uIGBNQVhfUkVTUE9OU0VfSVRFTVNgKQ==-->package cleanup rules are not applied when there are more than 200 packages (depends on `MAX_RESPONSE_ITEMS`)<!--description--> <!--end release-notes-assistant--> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9219 Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org> Co-authored-by: Michael Kriese <michael.kriese@visualon.de> Co-committed-by: Michael Kriese <michael.kriese@visualon.de>
This commit is contained in:
		
					parent
					
						
							
								7217965542
							
						
					
				
			
			
				commit
				
					
						c697de9517
					
				
			
		
					 3 changed files with 26 additions and 7 deletions
				
			
		| 
						 | 
					@ -9,7 +9,6 @@ import (
 | 
				
			||||||
	"net/http"
 | 
						"net/http"
 | 
				
			||||||
	"time"
 | 
						"time"
 | 
				
			||||||
 | 
					
 | 
				
			||||||
	"forgejo.org/models/db"
 | 
					 | 
				
			||||||
	packages_model "forgejo.org/models/packages"
 | 
						packages_model "forgejo.org/models/packages"
 | 
				
			||||||
	repo_model "forgejo.org/models/repo"
 | 
						repo_model "forgejo.org/models/repo"
 | 
				
			||||||
	user_model "forgejo.org/models/user"
 | 
						user_model "forgejo.org/models/user"
 | 
				
			||||||
| 
						 | 
					@ -168,13 +167,12 @@ func SetRulePreviewContext(ctx *context.Context, owner *user_model.User) {
 | 
				
			||||||
			PackageID:  p.ID,
 | 
								PackageID:  p.ID,
 | 
				
			||||||
			IsInternal: optional.Some(false),
 | 
								IsInternal: optional.Some(false),
 | 
				
			||||||
			Sort:       packages_model.SortCreatedDesc,
 | 
								Sort:       packages_model.SortCreatedDesc,
 | 
				
			||||||
			Paginator:  db.NewAbsoluteListOptions(pcr.KeepCount, 200),
 | 
					 | 
				
			||||||
		})
 | 
							})
 | 
				
			||||||
		if err != nil {
 | 
							if err != nil {
 | 
				
			||||||
			ctx.ServerError("SearchVersions", err)
 | 
								ctx.ServerError("SearchVersions", err)
 | 
				
			||||||
			return
 | 
								return
 | 
				
			||||||
		}
 | 
							}
 | 
				
			||||||
		for _, pv := range pvs {
 | 
							for _, pv := range pvs[pcr.KeepCount:] {
 | 
				
			||||||
			if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
 | 
								if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
 | 
				
			||||||
				ctx.ServerError("ShouldBeSkipped", err)
 | 
									ctx.ServerError("ShouldBeSkipped", err)
 | 
				
			||||||
				return
 | 
									return
 | 
				
			||||||
| 
						 | 
					
 | 
				
			||||||
| 
						 | 
					@ -64,13 +64,12 @@ func ExecuteCleanupRules(outerCtx context.Context) error {
 | 
				
			||||||
				PackageID:  p.ID,
 | 
									PackageID:  p.ID,
 | 
				
			||||||
				IsInternal: optional.Some(false),
 | 
									IsInternal: optional.Some(false),
 | 
				
			||||||
				Sort:       packages_model.SortCreatedDesc,
 | 
									Sort:       packages_model.SortCreatedDesc,
 | 
				
			||||||
				Paginator:  db.NewAbsoluteListOptions(pcr.KeepCount, 200),
 | 
					 | 
				
			||||||
			})
 | 
								})
 | 
				
			||||||
			if err != nil {
 | 
								if err != nil {
 | 
				
			||||||
				return fmt.Errorf("CleanupRule [%d]: SearchVersions failed: %w", pcr.ID, err)
 | 
									return fmt.Errorf("CleanupRule [%d]: SearchVersions failed: %w", pcr.ID, err)
 | 
				
			||||||
			}
 | 
								}
 | 
				
			||||||
			versionDeleted := false
 | 
								versionDeleted := false
 | 
				
			||||||
			for _, pv := range pvs {
 | 
								for _, pv := range pvs[pcr.KeepCount:] {
 | 
				
			||||||
				if pcr.Type == packages_model.TypeContainer {
 | 
									if pcr.Type == packages_model.TypeContainer {
 | 
				
			||||||
					if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
 | 
										if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
 | 
				
			||||||
						return fmt.Errorf("CleanupRule [%d]: container.ShouldBeSkipped failed: %w", pcr.ID, err)
 | 
											return fmt.Errorf("CleanupRule [%d]: container.ShouldBeSkipped failed: %w", pcr.ID, err)
 | 
				
			||||||
| 
						 | 
					
 | 
				
			||||||
| 
						 | 
					@ -19,9 +19,11 @@ import (
 | 
				
			||||||
	unit_model "forgejo.org/models/unit"
 | 
						unit_model "forgejo.org/models/unit"
 | 
				
			||||||
	"forgejo.org/models/unittest"
 | 
						"forgejo.org/models/unittest"
 | 
				
			||||||
	user_model "forgejo.org/models/user"
 | 
						user_model "forgejo.org/models/user"
 | 
				
			||||||
 | 
						"forgejo.org/modules/container"
 | 
				
			||||||
	"forgejo.org/modules/setting"
 | 
						"forgejo.org/modules/setting"
 | 
				
			||||||
	api "forgejo.org/modules/structs"
 | 
						api "forgejo.org/modules/structs"
 | 
				
			||||||
	"forgejo.org/modules/test"
 | 
						"forgejo.org/modules/test"
 | 
				
			||||||
 | 
						"forgejo.org/modules/translation"
 | 
				
			||||||
	"forgejo.org/modules/util"
 | 
						"forgejo.org/modules/util"
 | 
				
			||||||
	packages_service "forgejo.org/services/packages"
 | 
						packages_service "forgejo.org/services/packages"
 | 
				
			||||||
	packages_cleanup_service "forgejo.org/services/packages/cleanup"
 | 
						packages_cleanup_service "forgejo.org/services/packages/cleanup"
 | 
				
			||||||
| 
						 | 
					@ -535,6 +537,9 @@ func TestPackageCleanup(t *testing.T) {
 | 
				
			||||||
	t.Run("CleanupRules", func(t *testing.T) {
 | 
						t.Run("CleanupRules", func(t *testing.T) {
 | 
				
			||||||
		defer tests.PrintCurrentTest(t)()
 | 
							defer tests.PrintCurrentTest(t)()
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
							locale := translation.NewLocale("en-US")
 | 
				
			||||||
 | 
							session := loginUser(t, user.Name)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
		type version struct {
 | 
							type version struct {
 | 
				
			||||||
			Version     string
 | 
								Version     string
 | 
				
			||||||
			ShouldExist bool
 | 
								ShouldExist bool
 | 
				
			||||||
| 
						 | 
					@ -656,17 +661,34 @@ func TestPackageCleanup(t *testing.T) {
 | 
				
			||||||
				pcr, err := packages_model.InsertCleanupRule(db.DefaultContext, c.Rule)
 | 
									pcr, err := packages_model.InsertCleanupRule(db.DefaultContext, c.Rule)
 | 
				
			||||||
				require.NoError(t, err)
 | 
									require.NoError(t, err)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
									req := NewRequest(t, "GET", fmt.Sprintf("/user/settings/packages/rules/%d/preview", pcr.ID))
 | 
				
			||||||
 | 
									resp := session.MakeRequest(t, req, http.StatusOK)
 | 
				
			||||||
 | 
									htmlDoc := NewHTMLParser(t, resp.Body)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
									toDelete := container.FilterSlice(c.Versions, func(v version) (version, bool) {
 | 
				
			||||||
 | 
										return v, !v.ShouldExist
 | 
				
			||||||
 | 
									})
 | 
				
			||||||
 | 
									deletedCount := len(toDelete)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
									// disabled rule would delete everything
 | 
				
			||||||
 | 
									if !pcr.Enabled {
 | 
				
			||||||
 | 
										deletedCount = 1
 | 
				
			||||||
 | 
										htmlDoc.AssertSelection(t, htmlDoc.FindByText(fmt.Sprintf("a[href='/%s/-/packages/generic/package/keep']", user.Name), "keep"), true)
 | 
				
			||||||
 | 
									}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
									htmlDoc.AssertSelection(t, htmlDoc.FindByText("p", locale.TrString("packages.owner.settings.cleanuprules.preview.overview", deletedCount)), true)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
				err = packages_cleanup_service.CleanupTask(db.DefaultContext, duration)
 | 
									err = packages_cleanup_service.CleanupTask(db.DefaultContext, duration)
 | 
				
			||||||
				require.NoError(t, err)
 | 
									require.NoError(t, err)
 | 
				
			||||||
 | 
					
 | 
				
			||||||
				for _, v := range c.Versions {
 | 
									for _, v := range c.Versions {
 | 
				
			||||||
					pv, err := packages_model.GetVersionByNameAndVersion(db.DefaultContext, user.ID, packages_model.TypeGeneric, "package", v.Version)
 | 
										pv, err := packages_model.GetVersionByNameAndVersion(db.DefaultContext, user.ID, packages_model.TypeGeneric, "package", v.Version)
 | 
				
			||||||
					if v.ShouldExist {
 | 
										if v.ShouldExist {
 | 
				
			||||||
						require.NoError(t, err)
 | 
											require.NoError(t, err, `version "%s" should exist`, v.Version)
 | 
				
			||||||
						err = packages_service.DeletePackageVersionAndReferences(db.DefaultContext, pv)
 | 
											err = packages_service.DeletePackageVersionAndReferences(db.DefaultContext, pv)
 | 
				
			||||||
						require.NoError(t, err)
 | 
											require.NoError(t, err)
 | 
				
			||||||
					} else {
 | 
										} else {
 | 
				
			||||||
						require.ErrorIs(t, err, packages_model.ErrPackageNotExist)
 | 
											require.ErrorIs(t, err, packages_model.ErrPackageNotExist, `version "%s" should not exist`, v.Version)
 | 
				
			||||||
					}
 | 
										}
 | 
				
			||||||
				}
 | 
									}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
| 
						 | 
					
 | 
				
			||||||
		Loading…
	
	Add table
		Add a link
		
	
		Reference in a new issue