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

Data mover micro service backup #8046

Merged
merged 3 commits into from
Jul 30, 2024

Conversation

Lyndon-Li
Copy link
Contributor

Data mover micro service backup according to design #7576

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 61.70213% with 108 lines in your changes missing coverage. Please review.

Project coverage is 58.91%. Comparing base (53b57f8) to head (6997b8e).
Report is 3 commits behind head on main.

Files Patch % Lines
pkg/cmd/cli/datamover/backup.go 43.26% 59 Missing ⚠️
pkg/datamover/backup_micro_service.go 71.42% 41 Missing and 3 partials ⚠️
pkg/util/kube/event.go 79.16% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8046      +/-   ##
==========================================
+ Coverage   58.88%   58.91%   +0.02%     
==========================================
  Files         351      353       +2     
  Lines       29367    29643     +276     
==========================================
+ Hits        17294    17464     +170     
- Misses      10639    10738      +99     
- Partials     1434     1441       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Lyndon-Li Lyndon-Li force-pushed the data-mover-ms-backup-1 branch 4 times, most recently from 3a04527 to 0786dbc Compare July 25, 2024 05:51
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li Lyndon-Li marked this pull request as ready for review July 25, 2024 06:15
return nil, errors.Wrap(err, "error to create client config")
}

ctrl.SetLogger(zap.New(zap.UseDevMode(true)))
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this line is not relevant to the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check this separately since this code is same with the node-agent server.
If there is any problem, I will modify it for both here and the node-agent server in a separate PR.

Copy link
Contributor Author

@Lyndon-Li Lyndon-Li Jul 30, 2024

Choose a reason for hiding this comment

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

After a preliminary check, this is basically OK.
zap.New(zap.UseDevMode(true))also initializes a logger tostderrwhich is the same withlogger logrus.FieldLoggerand the default log level isInfo. Merely, logger logrus.FieldLoggeris with more info about the log level and format, but zap.New(zap.UseDevMode(true))may not comply with them. We probably seldomly need the logs from controller-runtime to be indebug` level.

The same code in the node-agent server was introduced in this PR #2561. Since it is an old one, we don't know any other special reason for not using the provided logger.

Therefore, I suggest we keep it as is until we see problems, then we come back to change all the places the same as here.

@Lyndon-Li Lyndon-Li merged commit 8e0f4d1 into vmware-tanzu:main Jul 30, 2024
45 checks passed
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.

3 participants