Skip to content

Conversation

@Lyndon-Li
Copy link
Contributor

Data mover micro service backup according to design #7576

@codecov
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
@Lyndon-Li Lyndon-Li force-pushed the data-mover-ms-backup-1 branch from 0786dbc to 8742f1b Compare July 25, 2024 06:03
@Lyndon-Li Lyndon-Li marked this pull request as ready for review July 25, 2024 06:15
@github-actions github-actions bot requested review from blackpiglet and sseago 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.

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