Skip to content
This repository was archived by the owner on Aug 30, 2024. It is now read-only.

Migrate to cobra #86

Merged
merged 10 commits into from
Aug 10, 2020
Merged

Migrate to cobra #86

merged 10 commits into from
Aug 10, 2020

Conversation

cmoog
Copy link
Contributor

@cmoog cmoog commented Jul 31, 2020 •
edited
Loading

I plan on following this up immediately with a cleanup of the error handling across the CLI layer. I'd prefer to merge as soon as posable to prevent blocking additional work. We should move away from calling flog.Fatal and towards the built-in error handling present in the urfave/cli. Done.

As noted by @nhooyr, the closure pattern is non-obvious. Let me know what you think.

This PR also includes many improvements/additions, most notably to the envs and urls subcommands. urls was kept at the top level to prevent unnecessary coupling, but I'm thinking we should move it to a subcommand of envs.

coder provides a CLI for working with an existing Coder Enterprise installation

Usage:
  coder [command]

Available Commands:
  completion  Generate completion script
  config-ssh  Configure SSH to access Coder environments
  envs        Interact with Coder environments
  help        Help about any command
  login       Authenticate this client for future operations
  logout      Remove local authentication credentials if any exist
  secrets     Interact with Coder Secrets
  sh          Open a shell and execute commands in a Coder environment
  sync        Establish a one way directory sync to a Coder environment
  urls        Interact with environment DevURLs
  users       Interact with Coder user accounts

Flags:
  -h, --help      help for coder
  -v, --version   version for coder

Use "coder [command] --help" for more information about a command.

Closes #85
cc @nhooyr @anthonyshull @coadler

Closes #82

nhooyr
nhooyr reviewed Aug 4, 2020
View reviewed changes
cmd/coder/secrets.go Outdated
}
}

type listSecretsCmd struct{}
func makeCreateSecret() cli.Command {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk how I feel about this.

@cmoog cmoog force-pushed the urfave branch 2 times, most recently from b9cb39d to 316033e Compare August 4, 2020 16:07
@cmoog cmoog mentioned this pull request Aug 4, 2020
Closed
@cmoog cmoog marked this pull request as ready for review August 4, 2020 16:50
@cmoog cmoog requested review from coadler and scsmithr August 4, 2020 20:36
coadler
coadler suggested changes Aug 5, 2020
View reviewed changes
@nhooyr
Copy link
Contributor

nhooyr commented Aug 5, 2020

Do you prefer v2 or v1 urfave @cmoog?

@coadler
Copy link
Contributor

coadler commented Aug 5, 2020

From looking that the migration docs it just seems like they made envs accept a slice and made most of the command/flag structs pointers instead.

@cmoog
Copy link
Contributor Author

cmoog commented Aug 6, 2020

It looks like they enforce that flags are positioned before arguments too. Definetly not a fan of that. For example, the following is invalid.

coder secrets create newsecret --from-literal 123

In v2, this must be

coder secrets create --from-literal 123 newsecret

@nhooyr
Copy link
Contributor

nhooyr commented Aug 6, 2020

Yea that straight sucks :(

@cmoog
Copy link
Contributor Author

cmoog commented Aug 6, 2020 •
edited
Loading

Yeah for sure, going to revert the migration cause that's super annoying.

@nhooyr
Copy link
Contributor

nhooyr commented Aug 6, 2020

I’d be in favour of using cobra then.

@cmoog
Copy link
Contributor Author

cmoog commented Aug 6, 2020

Nevermind, I think the flag-before-arg is fine given that the help template accurately reflects that and we'll show plenty of example usages in the documentation.

@cmoog cmoog requested a review from coadler August 6, 2020 18:52
@nhooyr
Copy link
Contributor

nhooyr commented Aug 6, 2020

From a UX perspective it's super annoying imo when I'm editing a CLI command. Any context on why they made this change?

@coadler
Copy link
Contributor

coadler commented Aug 7, 2020

@nhooyr https://github.com/urfave/cli/blob/master/docs/migrate-v1-to-v2.md#flags-before-args

In v2 flags must come before args. This is more POSIX-compliant.

@nhooyr
Copy link
Contributor

nhooyr commented Aug 7, 2020

That’s so dumb. What does it have to do with POSIX lol.

@nhooyr
Copy link
Contributor

nhooyr commented Aug 7, 2020

urfave/cli#1113

@cmoog
Copy link
Contributor Author

cmoog commented Aug 7, 2020

@nhooyr @coadler how do you cast your votes?

@nhooyr
Copy link
Contributor

nhooyr commented Aug 7, 2020

If you're ok migrating, I'd say we go for cobra instead for UX. But I defer to you.

@cmoog cmoog changed the title Migrate to urfave/cli Migrate to cobra Aug 7, 2020
@cmoog cmoog removed the request for review from scsmithr August 7, 2020 17:30
@cmoog
Copy link
Contributor Author

cmoog commented Aug 7, 2020

There are some error handling inconsistencies but nothing too bad that can't be fixed after this merges. Don't want to keep this unmerged much longer as it's blocking feature work.

nhooyr
nhooyr reviewed Aug 7, 2020
View reviewed changes
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how's the fish completion now?

ci/README.md
@@ -0,0 +1,26 @@
# ci
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to link to this from the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most readers of the README will be customers, not developers. Doesn't really make sense to bring attention to it in my view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have to bring attention to it. Just put a link at the bottom. Customers/potential customers who care to look closely will be happy we document and care about these things.

@cmoog cmoog requested a review from nhooyr August 10, 2020 14:36
@nhooyr
Copy link
Contributor

nhooyr commented Aug 10, 2020

I'm a little swamped right now, don't think I can give this a good review sorry!

@cmoog cmoog removed the request for review from nhooyr August 10, 2020 18:39
@cmoog
Copy link
Contributor Author

cmoog commented Aug 10, 2020

To unblock feature work and prevent a rush before the release date, I'm going to merge this now. Some cleanup PRs will be following in parallel.

@cmoog cmoog merged commit 56a76c4 into master Aug 10, 2020
@cmoog cmoog deleted the urfave branch August 10, 2020 21:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nhooyr nhooyr nhooyr left review comments

@coadler coadler Awaiting requested review from coadler

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate to urfave/cli Running 'coder urls' without any arguments errors
3 participants
@cmoog @nhooyr @coadler