Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Aug 30, 2024. It is now read-only.
/coder-v1-cliPublic archive

Add custom prefix devurl#74

Merged
Russtopia merged 7 commits intomasterfrom4411-coder-cli-named-devurls
Aug 4, 2020
Merged

Conversation

Russtopia
Copy link

@RusstopiaRusstopia commentedJul 15, 2020
edited
Loading

This implementshttps://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 withhttps://github.com/cdr/enterprise/pull/4381

What This Does

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

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

  • REST APIs to manipulate devurls have theport argument changed to int type

Testing

Usage Examples

  • TODO

@RusstopiaRusstopiaforce-pushed the4411-coder-cli-named-devurls branch from9596c44 to3a9cfc4CompareJuly 17, 2020 17:05
@RusstopiaRusstopiaforce-pushed the4411-coder-cli-named-devurls branch from3a9cfc4 tofa8cbf7CompareJuly 17, 2020 17:46
@RusstopiaRusstopia linked an issueJul 20, 2020 that may beclosed by this pull request
@scsmithrscsmithr requested a review fromcoadlerJuly 21, 2020 14:47
@Russtopia
Copy link
Author

Russtopia commentedJul 21, 2020
edited
Loading

In general: how do we feel about thecreate andedit subcommands (versus justcreate in master)?
I feel it's more sensible from a user standpoint to make separate verbs.. and non-intuitive thatcreate could alsoupdate an existing devurl. If no one has objections, I'd like to makecreate actually refuse to update an existing devurl, and letedit do that.

The downside is thatcreate 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 tocreate e.g.

$ coder create -m <env> <port>

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

Copy link
Contributor

@scsmithrscsmithr 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 justcreate. Personally, I like that it's idempotent(ish) and I'd find it annoying to have to calledit in the case of me forgetting that a devurl already exists. I like my CLIs to just do what I mean.

@RusstopiaRusstopiaforce-pushed the4411-coder-cli-named-devurls branch 2 times, most recently fromea7e23a to3ced5b2CompareJuly 28, 2020 19:36
@RusstopiaRusstopiaforce-pushed the4411-coder-cli-named-devurls branch from3ced5b2 tob4109bfCompareJuly 28, 2020 19:41
@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 justcreate. Personally, I like that it's idempotent(ish) and I'd find it annoying to have to calledit 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 usecreate for both ops.

@RusstopiaRusstopiaforce-pushed the4411-coder-cli-named-devurls branch fromb4109bf to5d92778CompareJuly 30, 2020 15:24
Copy link
Contributor

@cmoogcmoog 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.

Russtopia reacted with thumbs up emoji
@RusstopiaRusstopiaforce-pushed the4411-coder-cli-named-devurls branch from7924bb3 tocc55a4aCompareJuly 30, 2020 22:23
@RusstopiaRusstopia requested a review fromcmoogJuly 30, 2020 22:27
@@ -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

@RusstopiaRusstopiaJul 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 theif err != nil { exitUsage(f1) } right into those two helpers it would simplify the flow here. However, to callexitUsage(fl) within, I'd have to passfl to both helpers which seems icky so perhaps I'll just leave these alone.

@RusstopiaRusstopia requested a review fromcmoogJuly 31, 2020 17:56
Copy link
Contributor

@cmoogcmoog 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

@Russtopia Anything blocking this from merging?

@RusstopiaRusstopia merged commitbae77f0 intomasterAug 4, 2020
@cmoogcmoog deleted the 4411-coder-cli-named-devurls branchAugust 5, 2020 14:24
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@coadlercoadlercoadler approved these changes

@cmoogcmoogcmoog approved these changes

@anthonyshullanthonyshullAwaiting requested review from anthonyshull

@scsmithrscsmithrAwaiting 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

[8]ページ先頭

©2009-2025 Movatter.jp