Fix missing defaultVolumesToFsBackup flag output in Velero describe backup cmd

[shubham-pampattiwar]

Thank you for contributing to Velero!

Please add a summary of your change

Add missing defaultVolumesToFSBackup flag info to Velero describe cmd

Does your change fix a particular issue?

Fixes #9055

Please indicate you've done the following:

• Accepted the DCO. Commits without the DCO will delay acceptance.
• Created a changelog file (make new-changelog) or comment /kind changelog-not-required on this PR.
• Updated the corresponding documentation in site/content/docs/main.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.06%. Comparing base (232bc90) to head (b551e4c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9056   +/-   ##
=======================================
  Coverage   60.06%   60.06%           
=======================================
  Files         379      379           
  Lines       43483    43486    +3     
=======================================
+ Hits        26116    26119    +3     
  Misses      15801    15801           
  Partials     1566     1566           

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

🚀 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.

[reasonerjt]

1. I think we need better wording to this field rather than "Default Volumes to Fs Backup". cc @Lyndon-Li
2. I don't think "auto" is a valid value of this field, it has to be either "true" or "false"

[shubham-pampattiwar]

1. How about Pod Volume Backup (Default): or File System Backup (Default) ?
2. How about "<Not Set>"

[sseago]

@shubham-pampattiwar It will only ever be "not set" before the backup controller reconciles it. If it's not set, then the default value from the server gets copied over. But that is a valid use case for describe, since a backup will be in that state for a while if another backup is currently running when it's created.

[Lyndon-Li]

I am somehow confused with the purpose:

1. If we want to reflect the backup method, we don't need this field at all, because as @sseago mentioned, we don't know the backup method until the backup controller reconciles it; while, we already have this info in the volume info section.
2. If we want to show backup options selected by users, it is enough to make a straight as:
   Showing "Default to fs-backup" when default-to-fs-backup is true
   Showing nothing otherwise

[shubham-pampattiwar]

@Lyndon-Li Thats what the PR currently does, so the point of conflict is the usage of "auto" when its not set, right ? I see "auto" being already used for feilds like Velero-Native Snapshot PVs: in describe.

[Lyndon-Li]

@shubham-pampattiwar I mean which is the purpose for this PR, 1 or 2?
If the purpose is 1, I don't think we need to add this change.
If the purpose is 2, we just need to show it when default-to-fs-backup is set.

[shubham-pampattiwar]

@Lyndon-Li the purpose is option 2

[sseago]

@shubham-pampattiwar I'm not sure "auto" makes sense here. We use that for includeClusterResources since "auto" describes the behavior -- it's an intermediate state between "include all" and "exclude all" -- "include relevant ones only". But for defaultVolumesToFsBackup, nil just means "the controller hasn't filled in the server default value yet", which is a completely different meaning. Maybe "server default" instead of "auto" here? (in any case, once it's reconciled, it will be filled in with true or false)

[shubham-pampattiwar]

Updated the PR to only describe/show it when default-to-fs-backup is set by the user.

[kaovilai]

    --- FAIL: TestDescribeBackupSpec/old_resource_filter_with_hooks_and_ordered_resources (0.00s)
        backup_describer_test.go:393: 
            	Error Trace:	/github.com/vmware-tanzu/velero/pkg/cmd/util/output/backup_describer_test.go:393
            	Error:      	Not equal: 
            	            	expected: "Namespaces:\n  Included:  *\n  Excluded:  <none>\n\nResources:\n  Included:        *\n  Excluded:        <none>\n  Cluster-scoped:  auto\n\nLabel selector:  <none>\n\nOr label selector:  <none>\n\nStorage Location:  backup-location\n\nVelero-Native Snapshot PVs:    auto\nSnapshot Move Data:            auto\nData Mover:                    velero\n\nTTL:  0s\n\nCSISnapshotTimeout:    0s\nItemOperationTimeout:  0s\n\nHooks:\n  Resources:\n    hook-1:\n      Namespaces:\n        Included:  *\n        Excluded:  <none>\n\n      Resources:\n        Included:  *\n        Excluded:  <none>\n\n      Label selector:  <none>\n\n      Pre Exec Hook:\n        Container:  hook-container-1\n        Command:    pre\n        On Error:   Continue\n        Timeout:    0s\n\n      Post Exec Hook:\n        Container:  hook-container-1\n        Command:    post\n        On Error:   Continue\n        Timeout:    0s\n\nOrderedResources:\n  kind1: rs1-1, rs1-2\n"
            	            	actual  : "Namespaces:\n  Included:  *\n  Excluded:  <none>\n\nResources:\n  Included:        *\n  Excluded:        <none>\n  Cluster-scoped:  auto\n\nLabel selector:  <none>\n\nOr label selector:  <none>\n\nStorage Location:  backup-location\n\nVelero-Native Snapshot PVs:  auto\nSnapshot Move Data:          auto\nData Mover:                  velero\n\nTTL:  0s\n\nCSISnapshotTimeout:    0s\nItemOperationTimeout:  0s\n\nHooks:\n  Resources:\n    hook-1:\n      Namespaces:\n        Included:  *\n        Excluded:  <none>\n\n      Resources:\n        Included:  *\n        Excluded:  <none>\n\n      Label selector:  <none>\n\n      Pre Exec Hook:\n        Container:  hook-container-1\n        Command:    pre\n        On Error:   Continue\n        Timeout:    0s\n\n      Post Exec Hook:\n        Container:  hook-container-1\n        Command:    post\n        On Error:   Continue\n        Timeout:    0s\n\nOrderedResources:\n  kind1: rs1-1, rs1-2\n"

[reasonerjt]

@shubham-pampattiwar

I think File System Backup (Default) looks better than Default Volumes to Fs Backup.

cc @sseago @Lyndon-Li thoughts?

[Lyndon-Li]

@shubham-pampattiwar

I think File System Backup (Default) looks better than Default Volumes to Fs Backup.

cc @sseago @Lyndon-Li thoughts?

Yes, File System Backup (Default) should be better, there is no appearance of Fs Backup in our document.