Skip to content

cmd: restore-repo --units is repeated #19935

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

Closed
wants to merge 2 commits into from

Conversation

realaravinth
Copy link
Contributor

It is not a coma separated value. Add tests and fix the documentation.


This PR originates from @dachary of the forgefriends project. Please see here for origin.

It is not a coma separated value. Add tests and fix the documentation.
@realaravinth realaravinth marked this pull request as draft June 10, 2022 12:56
@realaravinth realaravinth marked this pull request as ready for review June 10, 2022 13:55
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 12, 2022
@Gusted Gusted added this to the 1.17.0 milestone Jun 12, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 12, 2022
"pull_requests",
"comments",
}

Copy link
Contributor

@wxiaoguang wxiaoguang Jun 12, 2022

Choose a reason for hiding this comment

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

I think this default value is not ideal for the server code.

The default value should be 0-length slice to indicate all units (there may be more in the future).

update: if it guarantees that the units always contains the units to be restored, the support for len(units) == 0 should be deleted to make the logic consistent.

image

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good idea to reduce the possibility to forget the places when we support more units.

Usage: `Which items will be restored, one or more units should be separated as comma.
wiki, issues, labels, releases, release_assets, milestones, pull_requests, comments are allowed. Empty means all units.`,
Value: &defaultUnits,
Usage: "Which items will be restored, can be repeated",
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
Usage: "Which items will be restored, can be repeated",
Usage: "Which items (wiki, issues, labels, releases, release_assets, milestones, pull_requests, comments) will be restored, can be repeated. Not providing this flag means all units.",

@wxiaoguang
Copy link
Contributor

Since it works, no blocker from my side. Just put my thoughts here.

@realaravinth
Copy link
Contributor Author

This pull request is closed as requested by the author who will not be able to answer further questions here because they do not have a GitHub account.

@lunny lunny removed this from the 1.17.0 milestone Jun 12, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants