Skip to content

Allow disable audit log to DB when initialize #22350

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow disable audit log to DB when initialize #22350

wants to merge 1 commit into from

Conversation

MinerYang
Copy link
Contributor

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Issue being fixed

Fixes #(22257)

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@MinerYang MinerYang requested a review from a team as a code owner September 12, 2025 13:24
@codecov Codecov
Copy link

codecov bot commented Sep 12, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.68%. Comparing base (c8c11b4) to head (342b081).
⚠️ Report is 568 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #22350      +/-   ##
==========================================
+ Coverage   45.36%   46.68%   +1.31%     
==========================================
  Files         244      252       +8     
  Lines       13333    14252     +919     
  Branches     2719     2927     +208     
==========================================
+ Hits         6049     6653     +604     
- Misses       6983     7244     +261     
- Partials      301      355      +54
Flag Coverage Δ
unittests 46.68% <ø> (+1.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 178 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MinerYang MinerYang force-pushed the init-skip-auditlog-db branch from b2e51b9 to 1d4fa4e Compare September 12, 2025 13:29
@MinerYang MinerYang added release-note/enhancement Label to mark PR to be added under release notes as enhancement target/2.14.1 labels Sep 12, 2025
@MinerYang MinerYang force-pushed the init-skip-auditlog-db branch 3 times, most recently from 5bfa645 to 177b920 Compare September 19, 2025 13:05
wy65701436
wy65701436 reviewed Sep 22, 2025
View reviewed changes
wy65701436
wy65701436 reviewed Sep 22, 2025
View reviewed changes
src/core/main.go

// get from db
mgr := config.GetCfgManager(ctx)
cfg, err := mgr.GetItemFromDriver(ctx, common.SkipAuditLogDatabase)
Copy link
Contributor

Choose a reason for hiding this comment

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

It can use the mgr.Get(), if the value is not set, this method will return a pre-defined error.

Copy link
Contributor Author

@MinerYang MinerYang Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

mgr.Get() get the k-v from the store map instead of the db driver directly. And it will always has a default values once cfgValues loaded

Copy link
Contributor

Choose a reason for hiding this comment

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

You should implement the function to check if k = common.SkipAuditLogDatabase exists in the properties table. not GetItermFromDriver()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure I could implement either check the key existence in the db or like now return k-v from db.
However I think get a k-v from db can be more generically used in the future compare to return the key existence bool value solely.

stonezdj
stonezdj reviewed Sep 22, 2025
View reviewed changes
src/core/main.go
func initSkipAuditDBbyEnv(ctx context.Context) error {
var err error
skipAuditEnv := false
s := os.Getenv("SKIP_LOG_AUDIT_DATABASE")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest to add _INIT suffix so that it can be self explained.

@MinerYang
Allow disable audit log to DB by Env when initialize
342b081 
Signed-off-by: my036811 <miner.yang@broadcom.com>
@MinerYang MinerYang force-pushed the init-skip-auditlog-db branch from 177b920 to 342b081 Compare September 23, 2025 14:39
@MinerYang
Copy link
Contributor Author

Hi @wy65701436 ,

Due to personal time break, please help to keep working on this issue. Appreciate!

Best,
Miner

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

@stonezdj stonezdj stonezdj left review comments

@wy65701436 wy65701436 wy65701436 left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@stonezdj stonezdj

@wy65701436 wy65701436

@chlins chlins

Labels
release-note/enhancement Label to mark PR to be added under release notes as enhancement target/2.14.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MinerYang @stonezdj @wy65701436 @chlins