Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

@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
@6543 6543 changed the title WIP: migrate urfave/cli to v2 version migrate urfave/cli to v2 version Apr 6, 2020
@6543 6543 force-pushed the update-vendor-cli branch from 7ce08bd to 3be0b4d Compare April 6, 2020 14:59
@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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
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