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

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

Open
AaronFeledy opened this issue Nov 12, 2021 · 9 comments

Comments

@AaronFeledy
Copy link
Member

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.

@AaronFeledy AaronFeledy added the bug Something isn't working label Nov 12, 2021
@pirog
Copy link
Member

pirog commented Nov 15, 2021

@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
Copy link
Member Author

@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
Copy link
Member

reynoldsalec commented Nov 16, 2021

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

@pirog
Copy link
Member

pirog commented Nov 16, 2021 via email

@reynoldsalec
Copy link
Member

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

@reynoldsalec
Copy link
Member

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: lando/legacy-cli#64

@pirog
Copy link
Member

pirog commented Nov 17, 2021

@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
Copy link

stale bot commented May 22, 2023

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
Copy link
Member Author

v4

@pirog pirog added Needs Triage and removed bug Something isn't working labels May 25, 2023
@pirog pirog transferred this issue from lando/lando May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants