Update backup log to reflect appropriate backup phase #7076
Conversation
shubham-pampattiwar
added
the
kind/changelog-not-required
pkg/controller/backup_controller.go
Outdated
| @@ -792,7 +792,7 @@ func (b *backupReconciler) runBackup(backup *pkgbackup.Request) error { | |||
| } | |||
| } | |||
|
|
|||
| b.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)).Info("Backup completed") | |||
| b.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)).Info(fmt.Sprintf("Initial backup processing complete, moving to %s", backup.Status.Phase)) | |||
There was a problem hiding this comment.
| b.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)).Info(fmt.Sprintf("Initial backup processing complete, moving to %s", backup.Status.Phase)) | |
| b.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)).Info(fmt.Sprintf("Initial backup processing completed, moving to %s", backup.Status.Phase)) |
There was a problem hiding this comment.
I think "complete" is better. i.e. "processing is complete" -- it avoids confusion with the "Completed" phase.
There was a problem hiding this comment.
+1 for complete
Codecov Report
@@ Coverage Diff @@
## main #7076 +/- ##
==========================================
+ Coverage 61.02% 61.13% +0.11%
==========================================
Files 255 256 +1
Lines 27040 27143 +103
==========================================
+ Hits 16500 16594 +94
- Misses 9361 9368 +7
- Partials 1179 1181 +2
|
pkg/controller/backup_controller.go
Outdated
| @@ -792,7 +792,7 @@ func (b *backupReconciler) runBackup(backup *pkgbackup.Request) error { | |||
| } | |||
| } | |||
|
|
|||
| b.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)).Info("Backup completed") | |||
| b.logger.WithField(Backup, kubeutil.NamespaceAndName(backup)).Info(fmt.Sprintf("Initial backup processing complete, moving to %s", backup.Status.Phase)) | |||
There was a problem hiding this comment.
Instead of fmt.Sprintf, can you use InfoF? (maybe aldo need to add line break \n)
There was a problem hiding this comment.
Infof is better than Sprintf into Info, although I don't think we generally include line breaks in log messages.
There was a problem hiding this comment.
Just worried that somethings add line break at the end (like fmt.Println), but others don't (like fmt.Printf), to do not have disorganized logs
There was a problem hiding this comment.
@mateusoliveira43 I'm not seeing any other log messages w/ \n in the codebase
There was a problem hiding this comment.
each log call should be single line. New line will always have timestamp in front.
There was a problem hiding this comment.
Thanks for the explanation, no line break needed 😄
There was a problem hiding this comment.
Done, Updated the PR @mateusoliveira43 @sseago @kaovilai
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> use infof instead of sprintf Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
shubham-pampattiwar
force-pushed
the
update-backup-log
branch
from
82d3674 to
ea7f249
Compare
Thank you for contributing to Velero!
Please add a summary of your change
The current
Backup Completedlog may not be appropriate for cases when there are some associated asynchronous plugin actions still being processed, so this PR updates the log to reflect the appropriate backup phase as per the ongoing backup process phase.Please indicate you've done the following:
/kind changelog-not-requiredas a comment on this pull request.site/content/docs/main.