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

Use consistent flag for setting auto idle on envs and projects #351

Open
tobybellwood opened this issue Jun 6, 2024 · 5 comments
Open
Milestone

Comments

@tobybellwood
Copy link
Member

The only place that auto-idle is used as an option is in the Environment command

if f.Name == "auto-idle" {

These should all be the same (ie autoIdle) IMO

@shreddedbacon
Copy link
Member

shreddedbacon commented Jun 6, 2024

I'm really against using camel case for flags, from a UX perspective having to switch case when typing out flags is annoying.
If we're going to be the same across commands, then they should all be auto-idle or autoidle.

Edit: also almost every other cli uses kebab case for flags too, so this would be my preference

@tobybellwood
Copy link
Member Author

😄 Just pointing out the discrepancy - happy for it to be kebab-case instead

Most of the project subcommands are still camelCase - should we alias them to kebab to avoid b/c issues?
https://uselagoon.github.io/lagoon-cli/commands/lagoon_add_project/
https://uselagoon.github.io/lagoon-cli/commands/lagoon_update_project/

@tobybellwood tobybellwood changed the title use consistent "autoIdle" option everywhere (instead of auto-idle on Env) Use consistent flag for setting auto idle on envs and projects Jun 6, 2024
@shreddedbacon
Copy link
Member

shreddedbacon commented Jun 6, 2024

Most of the project subcommands are still camelCase - should we alias them to kebab to avoid b/c issues? uselagoon.github.io/lagoon-cli/commands/lagoon_add_project uselagoon.github.io/lagoon-cli/commands/lagoon_update_project

The CLI is still not a v1 so we have some give in breaking change introduction. We can certainly double the flags and use stderr to warn about deprecated flags though probably (MarkDeprecated in cobra could do this)

Edit: yes it is possible to double flags and mark them deprecated

	addProjectCmd.Flags().UintP("autoIdle", "", 0, "Auto idle setting of the project")
	addProjectCmd.Flags().MarkDeprecated("autoIdle", "autoIdle is deprecated, use auto-idle")
	addProjectCmd.Flags().UintP("auto-idle", "a", 0, "Auto idle setting of the project")

result

$ go run main.go -l local add project -p projectah --gitUrl https://github.com/fake/repo --productionEnvironment main --openshift 2001 --autoIdle 0
Flag --gitUrl has been deprecated, gitUrl is deprecated, use git-url
Flag --autoIdle has been deprecated, autoIdle is deprecated, use auto-idle
Result: success
Project Name: projectah
GitURL: https://github.com/fake/repo

@tobybellwood
Copy link
Member Author

ugh - it gets a bit messy though with dual strings in AddProject - so the requiredInputCheck may need rewriting to support either git-url or gitUrl as an option - or we just deprecate and don't support dual flags.

Also, it looks like we just unmarshal the flags into the patch in updateProject, which isn't going to work if the flags don't match the GraphQL

@shreddedbacon
Copy link
Member

ugh - it gets a bit messy though with dual strings in AddProject - so the requiredInputCheck may need rewriting to support either git-url or gitUrl as an option - or we just deprecate and don't support dual flags.

Yeah, the requiredInputCheck function generally I don't think is actually required either, we have MarkFlagRequired in cobra that can do the this. But it still suffers from the problem of double flags.

subCmd.MarkFlagRequired("bar")

Also, it looks like we just unmarshal the flags into the patch in updateProject, which isn't going to work if the flags don't match the GraphQL

#321 fixes parts of that issue by moving to the newer machinery library instead of the legacy cli structure.


This is a solveable problem, and if we have to make some hacks in the requiredInputCheck to evaluate double flags for now then that is fine. Otherwise we take a breaking hit and people will have to adjust

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants