lando / core-next

Next generation Lando v4 runtime
https://docs.lando.dev/core/v4
GNU General Public License v3.0
3 stars 4 forks source link

Standardize on the --service argument for all tooling that can target a service #33

Open AaronFeledy opened 2 years ago

AaronFeledy commented 2 years ago

For just about everything in Lando that allows you to specify a service on which to perform an action, you would use the --service (-s) option. However, db-import uses --host for essentially the same effect. If a user (me just now) uses -s out of habit or expectation, the default database service for the recipe gets wiped out by the import since no --host was specified.

Lando should standardize on the --service argument for all tooling that can target a service.

pirog commented 2 years ago

@AaronFeledy i think the reason why we choose host instead of service for db-import|db-export is because it tracks with the actual argument you pass into the underlying sql command which IIRC is host. Not sure if that changes your opinion at all.

AaronFeledy commented 2 years ago

@pirog That's what I figured, but for the sake of consistency in both the usage and abstracting away the underlying tools, I think it should be --service.

reynoldsalec commented 2 years ago

Maybe best thing would be to add an alias, so both --host and --service would work?

pirog commented 2 years ago

I think we would have to do that for backwards compatibility reasons.

On Tue, Nov 16, 2021 at 6:39 PM Alec Reynolds @.***> wrote:

Maybe best thing would be to add an alias, so both --host or --service would work?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lando/core-next/issues/33, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFOFUAR2FSPLNZYJURRF33UMLTSPANCNFSM5H32RIHQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

reynoldsalec commented 2 years ago

Sweet, if you're ok with that approach @pirog I'll cook up a PR, seems trivial.

reynoldsalec commented 2 years ago

Huh, the approach I thought would work (adding alias options to the alias array in the tooling definition) didn't work. I'm guessing that there's a core section of Lando's CLI that makes some assumptions around the --service option that maybe I'm angering? @pirog if you think it's something easy to overcome LMK and I can tackle it.

Failed PR: https://github.com/lando/cli/pull/64

pirog commented 2 years ago

@reynoldsalec so currently the underlying API the the lando cli implements is YARGS and IIRC alias only allows for something like -f being an alias for --force. I think the implementation here would have to be a whole new option called --service and then having logic so that --host populates --service behind the scenes when--host is set and --service is not.

all that said, i personally think we shouldn't implement these kinds of changes in the current CLI but should probably just tag them as something to consider when we rebase on OCLIF as part of Lando 4

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions and please check out this if you are wondering why we auto close issues.

AaronFeledy commented 1 year ago

v4