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

migrate urfave/cli to v2 version #10912

Closed
wants to merge 55 commits into from
Closed

Conversation

6543
Copy link
Member

@6543 6543 commented Apr 1, 2020

main.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 1, 2020
@6543 6543 changed the title WIP: upgrade urfave/cli to v2 version WIP: migrate urfave/cli to v2 version Apr 1, 2020
main.go Outdated Show resolved Hide resolved
@6543 6543 changed the title WIP: migrate urfave/cli to v2 version migrate urfave/cli to v2 version Apr 6, 2020
@6543 6543 changed the title migrate urfave/cli to v2 version [WIP] migrate urfave/cli to v2 version Apr 6, 2020
@6543 6543 changed the title [WIP] migrate urfave/cli to v2 version migrate urfave/cli to v2 version May 16, 2020
@lafriks lafriks added this to the 1.13.0 milestone May 16, 2020
@stale stale bot added the issue/stale label May 1, 2022
@stale stale bot removed the issue/stale label May 13, 2022
@lunny
Copy link
Member

lunny commented May 16, 2022

CI failed seems related.

@6543
Copy link
Member Author

6543 commented May 16, 2022

Yes .. still same problem

(Internal binary exec call behave diffenent)

@wxiaoguang
Copy link
Contributor

IIRC, this PR is blocked by the different behavior for argument orders between urfave/cli v1 and v2 ?

https://github.com/urfave/cli/blob/main/docs/migrate-v1-to-v2.md#flags-before-args

It has been a long time, if it is the case, then there might be 2 choices:

  1. Introducing the breaking into Gitea for next release
  2. Keep urfave/cli.v1 , never go to v2

Correct me if I am wrong .....

@lunny
Copy link
Member

lunny commented May 25, 2022

It depends on how much benefit for Gitea migrate to v2.

@wxiaoguang
Copy link
Contributor

The design of urfave/cli.v2 is overall better than v1, except the flags must be before args rule. I can not understand why the cli author made such a strict rule (is there any way to bypass?)

@lunny
Copy link
Member

lunny commented May 25, 2022

How about my suggestion in #10912 (comment)

@jolheiser
Copy link
Member

jolheiser commented May 25, 2022

The design of urfave/cli.v2 is overall better than v1, except the flags must be before args rule. I can not understand why the cli author made such a strict rule (is there any way to bypass?)

POSIX-compliance is the quoted reason, and my guess is it makes parsing much less complex.


I am personally not a fan of cobra, so that's a 👎 from me. Many projects use it successfully, but I don't personally like it compared to urfave.

@6543
Copy link
Member Author

6543 commented May 25, 2022

I'm for change how internal git command create arguments - this will be breaking but should be fine if git hooks are recreated

@6543 6543 added pr/wip This PR is not ready for review pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! labels May 25, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 25, 2022

I'm for change how internal git command create arguments - this will be breaking but should be fine if git hooks are recreated

We can write a migration task to delete the Gitea binary path in AppState table, then Gitea will re-create all hooks automatically.


However, there are many scripts using Gitea binary, such change would break all a lot of them ..... I am still worrying about the breaking is too large. Maybe we should write a detailed document to evaluate the risk before spending coding time on it.

@zeripath
Copy link
Contributor

I presume that this args change doesn't mean that even subcommand args have to be before the subcommand?

If so looking at our docs we say:

gitea [global options] command [command or global options] [arguments...]

So whilst it's a breaking change it's more an undocumented change.

@lunny lunny modified the milestones: 1.17.0, 1.18.0 Jun 3, 2022
@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022
@lunny lunny modified the milestones: 1.19.0, 1.20.0 Jan 31, 2023
@GiteaBot GiteaBot removed this from the 1.20.0 milestone May 23, 2023
@wxiaoguang
Copy link
Contributor

-> Refactor to use urfave/cli/v2 #25959

lunny pushed a commit that referenced this pull request Jul 21, 2023
Replace #10912

And there are many new tests to cover the CLI behavior

There were some concerns about the "option order in hook scripts"
(#10912 (comment)),
it's not a problem now. Because the hook script uses `/gitea hook
--config=/app.ini pre-receive` format. The "config" is a global option,
it can appear anywhere.

----

## ⚠️ BREAKING ⚠️

This PR does it best to avoid breaking anything. The major changes are:

* `gitea` itself won't accept web's options: `--install-port` / `--pid`
/ `--port` / `--quiet` / `--verbose` .... They are `web` sub-command's
options.
    * Use `./gitea web --pid ....` instead
* `./gitea` can still run the `web` sub-command as shorthand, with
default options
* The sub-command's options must follow the sub-command
* Before: `./gitea --sub-opt subcmd` might equal to `./gitea subcmd
--sub-opt` (well, might not ...)
    * After: only `./gitea subcmd --sub-opt` could be used
    * The global options like `--config` are not affected
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 21, 2023
@6543 6543 deleted the update-vendor-cli branch November 22, 2023 00:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! pr/wip This PR is not ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants