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

Add warning/error result to cmd velero backup describe #5916

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

allenxu404
Copy link
Contributor

Thank you for contributing to Velero!

Please add a summary of your change

  • Add backupResulttype to downloadRequest crd
  • Add warning/error result to cmd velero backup describe

Does your change fix a particular issue?

Fixes #5200

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2023

Codecov Report

Merging #5916 (a0dac73) into main (1d8ca4f) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #5916      +/-   ##
==========================================
- Coverage   40.28%   40.15%   -0.13%     
==========================================
  Files         256      256              
  Lines       23342    23417      +75     
==========================================
  Hits         9403     9403              
- Misses      13225    13300      +75     
  Partials      714      714              
Impacted Files Coverage Δ
pkg/cmd/util/output/backup_describer.go 0.00% <0.00%> (ø)
pkg/cmd/util/output/backup_structured_describer.go 0.00% <0.00%> (ø)
pkg/cmd/util/output/restore_describer.go 0.00% <0.00%> (ø)
pkg/persistence/object_store.go 52.35% <0.00%> (-0.28%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sseago
Copy link
Collaborator

sseago commented Feb 25, 2023

Note that #5576 was recently merged that added backup warnings/errors to object store. From a quick look, it seems that this PR allows the newly-added results to be displayed via the backup describer. Let me know if I'm understanding the purpose here correctly and whether this PR adds onto that recently-merged result. In any case, I'll try to give this PR a more detailed look over the next week.

@allenxu404
Copy link
Contributor Author

@sseago Yes, this PR adds warning/error data to the output of backup describer and structured describer(a new describer developed in v1.11, which is used to display output in json format, please refer to #5865 for more info) based on #5576.

@anshulahuja98
Copy link
Collaborator

Thank you for this contribution @allenxu404 , I had intended to add this support post #5576
If possible, please try to share sample CLI outputs of this work.

I will meanwhile also review the PR further

@allenxu404 allenxu404 force-pushed the i5200 branch 2 times, most recently from 6f3a007 to c220891 Compare March 2, 2023 08:12
@allenxu404
Copy link
Contributor Author

allenxu404 commented Mar 2, 2023

@anshulahuja98 Here are some CLI output samples:

If errors/warnings exist && backupResults json not exists

velero backup describe myBackup:
Name: myBackup
Errors:    1
Warnings:  1
...

velero backup describe myBackup -o json:
{
    "name": "myBackup",
    "errors": {
        "count": 1
    },
    "warnings": {
        "count": 1
    },
    ...
}

If errors/warnings exist && backupResults json exists

velero backup describe myBackup:
Name: myBackup
Errors:
  Velero:     <none>
  Cluster:   cluster error
  Namespaces: <none>

Warnings:
  Velero:     <none>
  Cluster:   cluster warning
  Namespaces: <none>
...

velero backup describe myBackup -o json:
{
    "name": "myBackup",
    "errors": {
        "cluster": [
            "cluster error"
        ],
        "namespace": [],
        "velero": []
    },
    "warnings": {
        "cluster": [
            "cluster warning"
        ],
        "namespace": [],
        "velero": []
    },
    ...
}

@allenxu404
Copy link
Contributor Author

@sseago Do you have some time to review this PR?

@allenxu404
Copy link
Contributor Author

not very clear to me how this will end up looking like

for example for namespaced errors are you splitting the key/value pair to represent better?

Yes, the format is consistent with restore describe. Actually they share the same func.

@anshulahuja98
Copy link
Collaborator

I realized that later @allenxu404, and redacted that comment. Good that the function was reused.
I made some small nitpick comments, you can go through them. Otherwise the PR LGTM.

We would need maintainers to signoff. Good to see closure on the backupresults approach in a more user friendly way.
Thank you for your contribution.

sseago
sseago previously approved these changes Mar 6, 2023
sseago
sseago previously approved these changes Mar 8, 2023
Signed-off-by: allenxu404 <qix2@vmware.com>
@allenxu404
Copy link
Contributor Author

@sseago Can you give me another review? I merged the main branch and solved the conflicts.

@Lyndon-Li Lyndon-Li merged commit 117d5e8 into vmware-tanzu:main Mar 16, 2023
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.

error information enhancement for backup/restore
6 participants