toolshed/organising
7
1
Fork 0
Code Issues 102 Pull Requests Releases Activity

Non ambigous Argmuent and Flag position #581

New Issue
Closed
opened 2024-03-07 17:37:42 +00:00 by p4u1 · 13 comments
p4u1 commented 2024-03-07 17:37:42 +00:00
Owner
Copy Link
Copy Source

Let's take abra app cmd as an example, since we also have a real problem here

Currently the command usage is advertised the following:

USAGE:
   abra app command [command options] <domain> [<service>] <command> [-- <args>]

So I can run the following:

abra app cmd --local abra-test-recipe.local test_cmd_arg

But it is also possible to set [command options] after the like this:

abra app cmd abra-test-recipe.local test_cmd_arg --local

This behaviour was explicitly restored in this commit 0e688f1407 by downgrading our cli library urfave/cli from v2 to v1.

This had the consequence, that the following does not work anymore:

abra app cmd --local abra-test-recipe.local test_cmd_arg -- bing

I prose to upgrade urfave/cli back to v2 and there only allow command options directly after the command as it is also currently advertised.

In coop-cloud/abra#404 i implemented the upgrade, whats left is fixing all option usages in the integration tests. If accepted I am willing to do the rest of the work.

Let's take `abra app cmd` as an example, since we also have a real problem here Currently the command usage is advertised the following: ``` USAGE: abra app command [command options] <domain> [<service>] <command> [-- <args>] ``` So I can run the following: ``` abra app cmd --local abra-test-recipe.local test_cmd_arg ``` But it is also possible to set [command options] after the <command> like this: ``` abra app cmd abra-test-recipe.local test_cmd_arg --local ``` This behaviour was explicitly restored in this commit https://git.coopcloud.tech/coop-cloud/abra/commit/0e688f1407a1f17333ea6cf4c4f64ba9b9d800fa by downgrading our cli library `urfave/cli` from v2 to v1. This had the consequence, that the following does not work anymore: ``` abra app cmd --local abra-test-recipe.local test_cmd_arg -- bing ``` I prose to upgrade `urfave/cli` back to v2 and there only allow command options directly after the command as it is also currently advertised. In https://git.coopcloud.tech/coop-cloud/abra/pulls/404 i implemented the upgrade, whats left is fixing all option usages in the integration tests. If accepted I am willing to do the rest of the work.
p4u1 added the abra label 2024-03-07 17:37:42 +00:00
decentral1se commented 2024-03-08 07:40:17 +00:00
Owner
Copy Link
Copy Source

@p4u1 thanks for raising.

@3wordchant any thoughts?

@p4u1 thanks for raising. @3wordchant any thoughts?
basebuilder commented 2024-03-08 08:20:37 +00:00
Copy Link
Copy Source

My vote does not matter much (given I have zero deployments atm), but in terms of general codebase health- I thinking upgrading to urfave/cli#v2 is best. Release of v2.27.1 was in Dec, 2023 containing over 5 years and 100 smaller releases with what looks like many fixes and small improvements. They're already working on a v3. releases of cli 🤯

I have no idea how much this will screw up people's deployments and flows. Objectively speaking, I think this makes a truckload 🚚 🧠 more sense for the a longer term health of abra and Co-op Cloud to upgrade.

My vote does not matter much (given I have zero deployments atm), but in terms of general codebase health- I thinking upgrading to `urfave/cli#v2` is best. Release of `v2.27.1` was in Dec, 2023 containing over 5 years and 100 smaller releases with what looks like many fixes and small improvements. They're already working on a `v3.` releases of `cli` 🤯 I have no idea how much this will screw up people's deployments and flows. Objectively speaking, I think this makes a truckload 🚚 🧠 more sense for the a longer term health of `abra` and Co-op Cloud to upgrade.
👍 1
decentral1se commented 2024-03-09 08:51:46 +00:00
Owner
Copy Link
Copy Source

Additional data points:

From what I can remember in #284, we were forced in v2 to put [command options] only before the <command> and this was causing people issues. This is not "only allow command options directly after the command" @p4u1?

I've just built a local copy of coop-cloud/abra#404 and this is still the case:

🌻 ./abra app cmd --local abra-test-recipe.foo.org test_cmd_arg -- foo
foo
🌻 ./abra app cmd abra-test-recipe.foo.org test_cmd_arg --local -- foo
FATA[0000] abra-test-recipe doesn't have a --local function

I'm very confused again.

Additional data points: * https://git.coopcloud.tech/coop-cloud/organising/issues/284 * https://git.coopcloud.tech/coop-cloud/organising/issues/352 From what I can remember in #284, we were forced in v2 to put `[command options]` *only* before the `<command>` and this was causing people issues. This is not "only allow command options directly after the command" @p4u1? I've just built a local copy of https://git.coopcloud.tech/coop-cloud/abra/pulls/404 and this is still the case: > 🌻 ./abra app cmd --local abra-test-recipe.foo.org test_cmd_arg -- foo > foo > 🌻 ./abra app cmd abra-test-recipe.foo.org test_cmd_arg --local -- foo > FATA[0000] abra-test-recipe doesn't have a --local function I'm very confused again.
decentral1se commented 2024-03-09 08:55:06 +00:00
Owner
Copy Link
Copy Source

Annnnd this: coop-cloud/organising#361

@p4u1 v3 upgrade spike? 😱

Annnnd this: https://git.coopcloud.tech/coop-cloud/organising/issues/361 @p4u1 v3 upgrade spike? 😱
decentral1se commented 2024-03-11 11:30:37 +00:00
Owner
Copy Link
Copy Source

Update: we're leaving this in the new release as it's not a new regression and the fix for it needs some serious work. It's a rather minor bug which at least @moritz was able to figure out how to make work... very not ideal but we're so far behind on this release work, it seems like the best way forward now.

Update: we're leaving this in the new release as it's not a new regression and the fix for it needs some serious work. It's a rather minor bug which at least @moritz was able to figure out how to make work... very not ideal but we're so far behind on this release work, it seems like the best way forward now.
p4u1 referenced this issue from a commit 2024-03-11 13:27:21 +00:00
fix-integration-tests (!403)
p4u1 referenced this issue from a commit 2024-03-11 13:29:01 +00:00
fix-integration-tests (!403)
3wordchant commented 2024-03-22 18:10:26 +00:00
Owner
Copy Link
Copy Source

Maybe a survey of other CLI tools which support multiple word commands, to see which of them have restrictions on option placement, could be useful? And/or some UX testing on this?

In the mean-time, @p4u1, could you clarify what's not working in abra app cmd --local abra-test-recipe.local test_cmd_arg -- bing?

It seems like bing is meant to be passed to test_cmd_arg, and --local is meant to be handled by abra, correct? What is happening instead?

Maybe a survey of other CLI tools which support multiple word commands, to see which of them have restrictions on option placement, could be useful? And/or some UX testing on this? In the mean-time, @p4u1, could you clarify what's not working in `abra app cmd --local abra-test-recipe.local test_cmd_arg -- bing`? It seems like `bing` is meant to be passed to `test_cmd_arg`, and `--local` is meant to be handled by `abra`, correct? What is happening instead?
decentral1se added the bug label 2024-03-27 06:15:52 +00:00
decentral1se commented 2024-03-28 16:46:07 +00:00
Owner
Copy Link
Copy Source

@p4u1 and myself re-attempted to explain this to ourselves today... brain melter!

  • passing -- on the end as [-- ARGS] is broken, (-- gets parsed as an argument)
  • --local on the end, doesn't actually work:
    • the library parser doesn't include --local in the c.Args() if you pass it in the abra app cmd --local <domain> position (works!) but it is included if you pass it in the abra app cmd <domain> --local and this breaks code in abra that checks position of c.Args() for arguments. Since urfave/cli is generating abra app command command [command options] <domain> [<service>] <command> [-- <args>] as the help, it means we're actually somehow abusing the fact that options can be passed after commands? We believe this is why they released the "breaking change" in v2, which was just fixing this bug. Hence, forcing people to adhere to advertised usage?
  • seems to only (?) affects mostly abra app cmd because the interface is quite weird
@p4u1 and myself re-attempted to explain this to ourselves today... brain melter! * passing `--` on the end as `[-- ARGS]` is broken, (`--` gets parsed as an argument) * `--local` on the end, doesn't actually work: * the library parser doesn't include `--local` in the `c.Args()` if you pass it in the `abra app cmd --local <domain>` position (works!) but it *is* included if you pass it in the `abra app cmd <domain> --local` and this breaks code in `abra` that checks position of `c.Args()` for arguments. Since `urfave/cli` is generating `abra app command command [command options] <domain> [<service>] <command> [-- <args>]` as the help, it means we're actually somehow abusing the fact that options can be passed after commands? We believe this is why they released the "breaking change" in v2, which was just fixing this bug. Hence, forcing people to adhere to advertised usage? * seems to only (?) affects mostly `abra app cmd` because the interface is quite weird
decentral1se commented 2024-04-02 22:59:51 +00:00
Owner
Copy Link
Copy Source

https://docs.coopcloud.tech/abra/trouble/#command-line-flag-handling-is-weird

https://git.coopcloud.tech/coop-cloud/docs.coopcloud.tech/pulls/256/files#diff-cc94fdc55a0f6e6e79d71b15081836752766bb20

😱

https://docs.coopcloud.tech/abra/trouble/#command-line-flag-handling-is-weird https://git.coopcloud.tech/coop-cloud/docs.coopcloud.tech/pulls/256/files#diff-cc94fdc55a0f6e6e79d71b15081836752766bb20 😱
decentral1se commented 2024-07-08 15:38:18 +00:00
Owner
Copy Link
Copy Source

https://github.com/urfave/cli/pull/1928#issuecomment-2214466436

https://github.com/urfave/cli/pull/1928#issuecomment-2214466436
decentral1se commented 2024-07-15 09:27:05 +00:00
Owner
Copy Link
Copy Source

Update: someone tried to add the feature to urfave/cli but after a constant barrage of all the edge cases and ways in which it can go wrong, they gave up 😭 I'm now genuinely thinking of a spike to switch to cobra because after some testing with the hugo CLI UI/UX (which uses it), shit "just works".

Update: someone tried to add the feature to `urfave/cli` but after a constant barrage of all the edge cases and ways in which it can go wrong, they gave up 😭 I'm now genuinely thinking of a spike to switch to `cobra` because after some testing with the `hugo` CLI UI/UX (which uses it), shit "just works".
decentral1se commented 2024-07-16 14:27:56 +00:00
Owner
Copy Link
Copy Source

Hoping to kill this once and for all: coop-cloud/abra#435 (comment) (tldr; migrate to cobra)

Hoping to kill this once and for all: https://git.coopcloud.tech/coop-cloud/abra/pulls/435#issuecomment-21132 (tldr; migrate to `cobra`)
decentral1se commented 2024-10-21 14:52:42 +00:00
Owner
Copy Link
Copy Source

coop-cloud/abra#435

drunk style hackin' options everywhere is finally supported:

abra app cmd --chaos foo.com test_cmd_arg --local foo --debug

https://git.coopcloud.tech/coop-cloud/abra/pulls/435 drunk style hackin' options everywhere is finally supported: > `abra app cmd --chaos foo.com test_cmd_arg --local foo --debug`
🎉 2
decentral1se commented 2024-12-21 18:54:47 +00:00
Owner
Copy Link
Copy Source

We have new problems now. I notice now that the --help handling is broken, e.g. abra app cmd foo.com --help gives No help topic for 'foo.com' which is just broken. I'm going back to ripping out this CLI library, it is the source of all my morale destroying frustration.

We have new problems now. I notice now that the `--help` handling is broken, e.g. `abra app cmd foo.com --help` gives `No help topic for 'foo.com'` which is just broken. I'm going back to ripping out this CLI library, it is the source of all my morale destroying frustration.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
abra
Everything to do with abra
 awaiting-feedback
Ping/pong on comms
 backups
bug
Something is not working
 build
Go build related issues
 ci/cd
Getting the robots into the mix
 community organising
Opening this thing up
 contributing
Contributors stuff
 coopcloud.tech
Our main website
 design
Design thinking required
 documentation
Let's write things together
 duplicate
This issue or pull request already exists
 enhancement
New feature
 fedi
Democratic decision making
 fedi-infra
finance
Money things
 funding
Anything related to grant funding
 good first issue
Easy start with development
 help wanted
Need some help
 installer
Installation related issues
 legal
performance
Performance related
 proposal
Large change which requires feedback & decisin making
 question
More information is needed
 security
Securing our shit
 test
Unit or integration test suite
 wontfix
This won't be fixed
No labels abra bug
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
3wordchant aadil (Aadil Ayub) abra-bot (Abra Bot) ammaratef45 amras (Sarma) Apfelwurm BornDeleuze Brooke carla cas (Cassowary) coopcloud cyrnel decentral1se (d1) dede devydave fauno (fauno) iexos jade (Jade Ambrose) jjsfunhouse jmakdah2 (Jackie Makdah) joe-irving (Joe Irving) kawaiipunk (KawaiiPunk) knoflook kolaente lambdabundesverband linnealovespie (April) moosemower moritz notplants oxaliq (sorrel) p4u1 pharaohgraphy (Andrew 🐦‍🔥❤️‍🔥✴️) renovate-bot (Comrade Renovate Bot) ripclap simon sixsmith (Sixsmith) stevensting trav (Trav Fryer) val (val (he/him)) yksflip
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: toolshed/organising#581