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

Use controller-runtime's logging library #50

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Use controller-runtime's logging library #50

merged 3 commits into from
Oct 10, 2024

Conversation

toshipp
Copy link
Contributor

@toshipp toshipp commented Oct 9, 2024

We decided to use the controller-runtime's logging library rather than slog everywhere.

@toshipp toshipp changed the title Use logr Use controller-runtime's logging library Oct 9, 2024
@toshipp toshipp marked this pull request as draft October 9, 2024 07:43
@toshipp toshipp marked this pull request as ready for review October 9, 2024 09:13
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to get Ceph cluster ID: %s: %s: %w", mbc.Namespace, mbc.Spec.PVC, err)
}
if clusterID != r.managedCephClusterID {
logger.Info("the target pvc is not managed by this controller", "name", req.Name, "namespace", req.Namespace, "error", err)
logger.Info("the target pvc is not managed by this controller", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.Info("the target pvc is not managed by this controller", "error", err)
logger.Info("the target pvc is not managed by this controller")

err is always nil since error check was done in L113.

return ctrl.Result{}, err
}
restore.Status.Pool = pv.Spec.CSI.VolumeAttributes["pool"]
if restore.Status.Pool == "" {
logger.Error("pool not found in PV manifest", "backup", backup.Name, "namespace", backup.Namespace)
logger.Error(err, "pool not found in PV manifest", "backup", backup.Name, "namespace", backup.Namespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

err is nil since error check was done in L146. Please create a new error.

We used slog for logging in our codes, but controller-runtime uses its
own log system. Therefore, Mantle output logs with two different
formats. To avoid this, use the controller-runtime's library everywhere.

Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
controller-runtime sets a target object name into logger in default, it
is unenecessary.

Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
The error is determined as a nil, it is unnecessary to log it.

Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
@satoru-takeuchi satoru-takeuchi merged commit db9d03f into main Oct 10, 2024
3 checks passed
@satoru-takeuchi satoru-takeuchi deleted the use-logr branch October 10, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants