coop-cloud/organising
coop-cloud
/
organising
7
0
Fork 0
Code Issues 156 Pull Requests Projects 2 Releases Activity

Non ambigous Argmuent and Flag position #581

New Issue
Open
opened 2024-03-07 17:37:42 +00:00 by p4u1 · 8 comments
p4u1 commented 2024-03-07 17:37:42 +00:00
Member
Copy Link

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

@p4u1 thanks for raising.

@3wordchant any thoughts?

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

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

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

Annnnd this: #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

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

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

@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

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 😱
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
master
Labels
Clear labels   
abra

Everything to do with abra

  
abra-gandi

Abra Gandi plugin

  
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

  
democracy

Democratic decision making

  
design

Design thinking required

  
documentation

Let's write things together

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
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

  
kadabra

   
performance

Performance related

  
proposal

Large change which requires feedback & decisin making

  
question

More information is needed

  
recipes.coopcloud.tech

Our recipes catalogue

  
security

Securing our shit

  
test

Unit or integration test suite

  
wontfix

This won't be fixed

No Label
abra
abra-gandi
awaiting-feedback
backups
bug
build
ci/cd
community organising
contributing
coopcloud.tech
democracy
design
documentation
duplicate
enhancement
finance
funding
good first issue
help wanted
installer
kadabra
performance
proposal
question
recipes.coopcloud.tech
security
test
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: coop-cloud/organising#581
No description provided.
Delete Branch "%!s(<nil>)"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?