Skip to content
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.

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

feat: add new GC rules for release with version #6469

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

haiqingchq
Copy link
Contributor

What this PR does / why we need it:

Added a GC rule for releases with version on top of the original GC rule

Which issue(s) this PR fixes:

Specified Reviewers:

/assign @iutx @sfwn

ChangeLog

Language Changelog
🇺🇸 English add new GC rules for release with version
🇨🇳 中文 为带版本的制品添加新的 GC 规则

Need cherry-pick to release versions?

Add comment like /cherry-pick release/1.0 when this PR is merged.

For details on the cherry pick process, see the cherry pick requests section under CONTRIBUTING.md.

@erda-bot erda-bot requested review from iutx and sfwn November 21, 2024 08:26
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 45.00000% with 44 lines in your changes missing coverage. Please review.

Project coverage is 15.13%. Comparing base (d217119) to head (84ef097).

Files with missing lines Patch % Lines
...ternal/apps/dop/dicehub/release/release.service.go 43.63% 26 Missing and 5 partials ⚠️
internal/apps/dop/dicehub/release/db/release.go 57.14% 6 Missing and 3 partials ⚠️
internal/apps/dop/dicehub/release/provider.go 0.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6469      +/-   ##
==========================================
+ Coverage   15.12%   15.13%   +0.01%     
==========================================
  Files        3675     3675              
  Lines      377005   377085      +80     
==========================================
+ Hits        57018    57072      +54     
- Misses     314540   314551      +11     
- Partials     5447     5462      +15     
Flag Coverage Δ
by-github-actions 15.13% <45.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
internal/apps/dop/dicehub/release/provider.go 11.78% <0.00%> (-0.14%) ⬇️
internal/apps/dop/dicehub/release/db/release.go 10.95% <57.14%> (+7.24%) ⬆️
...ternal/apps/dop/dicehub/release/release.service.go 5.56% <43.63%> (+2.29%) ⬆️

... and 6 files with indirect coverage changes

Copy link

stale bot commented Dec 14, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 14, 2024
@sfwn
Copy link
Member

sfwn commented Dec 16, 2024

@iutx

@stale stale bot removed the wontfix label Dec 16, 2024
Copy link
Member

@iutx iutx left a comment

Choose a reason for hiding this comment

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

// ListReleaseByAppAndProject list release by application_id and project_id, version is not empty
func (client *ReleaseConfigDB) ListReleaseByAppAndProject(projectID int64, appID int64) ([]Release, error) {
var releases []Release
if err := client.Select([]string{"release_id", "cluster_name", "version"}).Where("application_id = ? AND project_id =? AND version != ''", appID, projectID).Order("updated_at ASC").Find(&releases).Error; err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Remove unnecessary code Select([]string{"release_id", "cluster_name", "version"}).

Copy link
Member

Choose a reason for hiding this comment

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

Format code in fragment link, e.g. Where, Order.

// ListExpireReleaseWithVersion list release that has not been referenced before a given point in time and version is not empty
func (client *ReleaseConfigDB) ListExpireReleaseWithVersion(projectID int64, applicationID int64, before time.Time) ([]Release, error) {
var releases []Release
if err := client.Select([]string{"release_id", "cluster_name", "version"}).Where("reference <= ?", 0).Where("updated_at < ?", before).Where("version != ''").
Copy link
Member

Choose a reason for hiding this comment

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

Remove unnecessary code fragment Select.

@@ -1055,6 +1055,59 @@ func (s *ReleaseService) GetLatestReleasesByProjectAndVersion(projectID int64, v
return &latests, nil
}

func (s *ReleaseService) FindExpiredReleaseBefore(now time.Time) ([]db.Release, error) {
// 7 days before
d, err := time.ParseDuration(strutil.Concat("-", "168", "h"))
Copy link
Member

Choose a reason for hiding this comment

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

Hard code is not recommend, move to the config file pls.

releaseCount := len(releases)
expiredCount := len(expiredReleases)

if releaseCount <= 10 {
Copy link
Member

Choose a reason for hiding this comment

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

As above comment.

if releaseCount <= 10 {
// If the quantity of release is less than 10, there is no need to delete
continue
} else if releaseCount < 30 {
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as above.


for i := range deleteRelease {
release := deleteRelease[i]
Copy link
Member

Choose a reason for hiding this comment

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

for _, v := is more clearly.

@@ -572,3 +574,86 @@ version: "2.0"
})
}
}

func TestReleaseServiceGC(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Pls use sqlmock instead SQLite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants