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

Introduce system artifact manager cleanup job #16879

Merged
merged 1 commit into from Jun 9, 2022

Conversation

prahaladdarkin
Copy link
Contributor

@prahaladdarkin prahaladdarkin commented May 19, 2022

Scope of change:

  • Introduce built-in job for system artifact clean-up.
  • Associated tests

Closes #16878

Signed-off-by: prahaladdarkin prahaladd@vmware.com

@prahaladdarkin prahaladdarkin requested a review from a team as a code owner May 19, 2022 06:28
@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #16879 (55b209e) into main (b8a71ac) will increase coverage by 0.01%.
The diff coverage is 73.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16879      +/-   ##
==========================================
+ Coverage   67.15%   67.17%   +0.01%     
==========================================
  Files         974      977       +3     
  Lines       81555    81644      +89     
  Branches     2599     2599              
==========================================
+ Hits        54767    54841      +74     
- Misses      23057    23068      +11     
- Partials     3731     3735       +4     
Flag Coverage Δ
unittests 67.17% <73.91%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
src/controller/systemartifact/callback.go 66.66% <66.66%> (ø)
src/controller/systemartifact/execution.go 70.14% <70.14%> (ø)
src/jobservice/job/impl/systemartifact/cleanup.go 86.66% <86.66%> (ø)
src/jobservice/runtime/bootstrap.go 50.32% <100.00%> (+0.32%) ⬆️
...audit-log-purge/history/purge-history.component.ts 49.01% <0.00%> (-9.81%) ⬇️
...es/vulnerability/vulnerability-config.component.ts 58.51% <0.00%> (+4.44%) ⬆️
...-job/gc-page/gc/gc-history/gc-history.component.ts 58.82% <0.00%> (+9.80%) ⬆️
src/lib/cache/util.go 89.47% <0.00%> (+15.78%) ⬆️


func init() {
if err := scheduler.RegisterCallbackFunc(SystemArtifactCleanupCallback, cleanupCallBack); err != nil {
log.Fatalf("failed to register the callback for the scan all schedule, error %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's for the system artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

func scheduleSystemArtifactCleanJob(ctx context.Context) {
cronSpec := "0 0 0 * * *"
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment for this, thanks.

}

func (c *Cleanup) ShouldRetry() bool {
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

refer to this #16833 (comment)

src/core/main.go Outdated Show resolved Hide resolved
@wy65701436 wy65701436 added the release-note/new-feature New Harbor Feature label May 31, 2022
Copy link
Member

@chlins chlins left a comment

Choose a reason for hiding this comment

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

lgtm

@prahaladdarkin prahaladdarkin force-pushed the pd_sysartifact_job branch 2 times, most recently from 25bfbdd to e217d79 Compare June 2, 2022 11:34
@@ -0,0 +1,128 @@
package systemartifact
Copy link
Contributor

Choose a reason for hiding this comment

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

the UT should cover all the methods, we need more, thanks.

@@ -0,0 +1,36 @@
package systemartifact
Copy link
Contributor

Choose a reason for hiding this comment

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

more UT if possible.

@prahaladdarkin prahaladdarkin force-pushed the pd_sysartifact_job branch 3 times, most recently from e3b6eef to a3d98da Compare June 8, 2022 16:38
Signed-off-by: prahaladdarkin <prahaladd@vmware.com>
Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@wy65701436 wy65701436 merged commit 4d062c3 into goharbor:main Jun 9, 2022
@prahaladdarkin prahaladdarkin deleted the pd_sysartifact_job branch July 17, 2022 16:49
sluetze pushed a commit to sluetze/harbor that referenced this pull request Oct 29, 2022
Signed-off-by: prahaladdarkin <prahaladd@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement System artifact cleanup job
3 participants