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

Add custom prefix devurl #74

Merged
merged 7 commits into from
Aug 4, 2020
Merged

Add custom prefix devurl #74

merged 7 commits into from
Aug 4, 2020

Conversation

Russtopia
Copy link

@Russtopia Russtopia commented Jul 15, 2020 •
edited
Loading

This implements https://github.com/cdr/enterprise/issues/4411
NOTE: this is intended for post 1.9 release. It involves a breaking API change to the devurl API and should be released in coordination with https://github.com/cdr/enterprise/pull/4381

What This Does

  • Adds new urls create subcommand option --name <label | "">
  • Adds new urls ls command and -o [human | json] options following syntax of the users ls command
  • Some error message output is modified to be more consistent (fmt.Prinf() vs. coder.com/flog)
  • Propose new edit urls subcommand which is just an alias for create; just for usage style
  • Propose to make create single-purpose (only create a new devurl) such that edit is the only way to modify an existing devurl
  • Scaffolding for integration tests of devurl operations

Changes for compatibility with https://github.com/cdr/enterprise/pull/4381

  • REST APIs to manipulate devurls have the port argument changed to int type

Testing

Usage Examples

  • TODO

Russtopia
Russtopia commented Jul 15, 2020
View reviewed changes
@Russtopia Russtopia force-pushed the 4411-coder-cli-named-devurls branch from 9596c44 to 3a9cfc4 Compare July 17, 2020 17:05
Russtopia
Russtopia commented Jul 17, 2020
View reviewed changes
@Russtopia Russtopia force-pushed the 4411-coder-cli-named-devurls branch from 3a9cfc4 to fa8cbf7 Compare July 17, 2020 17:46
@Russtopia Russtopia linked an issue Jul 20, 2020 that may be closed by this pull request
Add ability to specify custom devurl prefixes #73
Closed
@scsmithr scsmithr requested a review from coadler July 21, 2020 14:47
coadler
coadler reviewed Jul 21, 2020
View reviewed changes
@Russtopia
Copy link
Author

Russtopia commented Jul 21, 2020 •
edited
Loading

In general: how do we feel about the create and edit subcommands (versus just create in master)?
I feel it's more sensible from a user standpoint to make separate verbs.. and non-intuitive that create could also update an existing devurl. If no one has objections, I'd like to make create actually refuse to update an existing devurl, and let edit do that.

The downside is that create would then no longer be an idempotent operation; callers of coder-cli would need to take into account the state of the environment: whether a devurl already exists or not, in order to use the appropriate subcommand.

An alternative could be to add a flag to create e.g.

$ coder create -m <env> <port>

Where -m would mean 'create, or modify if already existing'. Not specifying -m would then mean create would refuse to modify existing devurls. We could keep edit as a separate modify-only subcommand, or do away with it.

scsmithr
scsmithr reviewed Jul 27, 2020
View reviewed changes
Copy link
Contributor

@scsmithr scsmithr left a comment

Choose a reason for hiding this comment

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

Regarding the create/edit subcommands, I'd prefer to either keep how you have it right now in the pr, or go back to having just create. Personally, I like that it's idempotent(ish) and I'd find it annoying to have to call edit in the case of me forgetting that a devurl already exists. I like my CLIs to just do what I mean.

@Russtopia Russtopia force-pushed the 4411-coder-cli-named-devurls branch 2 times, most recently from ea7e23a to 3ced5b2 Compare July 28, 2020 19:36
Russtopia
Russtopia commented Jul 28, 2020
View reviewed changes
@Russtopia Russtopia force-pushed the 4411-coder-cli-named-devurls branch from 3ced5b2 to b4109bf Compare July 28, 2020 19:41
@Russtopia Russtopia requested review from coadler and scsmithr July 28, 2020 19:56
@Russtopia
Copy link
Author

Regarding the create/edit subcommands, I'd prefer to either keep how you have it right now in the pr, or go back to having just create. Personally, I like that it's idempotent(ish) and I'd find it annoying to have to call edit in the case of me forgetting that a devurl already exists. I like my CLIs to just do what I mean.

Sure. Since @fuskovic added an alias ability to the cli recently, I left in "edit", but one can just use create for both ops.

@Russtopia Russtopia force-pushed the 4411-coder-cli-named-devurls branch from b4109bf to 5d92778 Compare July 30, 2020 15:24
cmoog
cmoog suggested changes Jul 30, 2020
View reviewed changes
Copy link
Contributor

@cmoog cmoog left a comment

Choose a reason for hiding this comment

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

Please add integration tests to ensure continued compatibility. #81 should be a good model.

cmoog
cmoog reviewed Jul 30, 2020
View reviewed changes
@Russtopia Russtopia force-pushed the 4411-coder-cli-named-devurls branch from 7924bb3 to cc55a4a Compare July 30, 2020 22:23
@Russtopia Russtopia requested a review from cmoog July 30, 2020 22:27
cmoog
cmoog suggested changes Jul 30, 2020
View reviewed changes
Russtopia
Russtopia commented Jul 31, 2020
View reviewed changes
cmd/coder/urls.go
@@ -137,35 +223,27 @@ func (sub delSubCmd) Run(fl *pflag.FlagSet) {
exitUsage(fl)
}

if !portIsValid(port) {
portNum, err := validatePort(port)
if err != nil {
exitUsage(fl)
}
Copy link
Author

@Russtopia Russtopia Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What's your preference on exit/fatal from 'helper' funcs?

Current way here prints out usage and exits with nonzero status which is what we'd want, I guess. If I tucked in the if err != nil { exitUsage(f1) } right into those two helpers it would simplify the flow here. However, to call exitUsage(fl) within, I'd have to pass fl to both helpers which seems icky so perhaps I'll just leave these alone.

@Russtopia Russtopia requested a review from cmoog July 31, 2020 17:56
cmoog
cmoog approved these changes Jul 31, 2020
View reviewed changes
Copy link
Contributor

@cmoog cmoog left a comment

Choose a reason for hiding this comment

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

Looks good to merge.

@cmoog
Copy link
Contributor

cmoog commented Aug 3, 2020

@Russtopia Anything blocking this from merging?

@Russtopia Russtopia merged commit bae77f0 into master Aug 4, 2020
@cmoog cmoog deleted the 4411-coder-cli-named-devurls branch August 5, 2020 14:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@coadler coadler coadler approved these changes

@cmoog cmoog cmoog approved these changes

@anthonyshull anthonyshull Awaiting requested review from anthonyshull

@scsmithr scsmithr Awaiting requested review from scsmithr

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

Successfully merging this pull request may close these issues.

Add ability to specify custom devurl prefixes
4 participants
@Russtopia @cmoog @scsmithr @coadler