- Notifications
You must be signed in to change notification settings - Fork1k
feat(cli): implement exp task status command#19533
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- Implements `coder exp task status`- Adds `testutil.MustURL` helper
cli/exp_task_status_test.go Outdated
| args []string | ||
| expectOutput string | ||
| expectError string | ||
| hf func(context.Context) func(http.ResponseWriter, *http.Request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: I elected to mock the API response instead of standing up a wholecoderdtest. We have too many of these as it is.
| "testing" | ||
| ) | ||
| func MustURL(t testing.TB, raw string) *url.URL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: I figured this might be useful.
cli/exp_task_status_test.go Outdated
| State: codersdk.TaskStateWorking, | ||
| }, | ||
| }) | ||
| c.Add(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm just sad I couldn't writec++ here 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it'd be nice to have a/status endpoint in coderd that allows follow without polling, but I'm fine with the current implementation for now. Some comments/suggestions inline.
Uh oh!
There was an error while loading.Please reload this page.
cli/exp_task_status.go Outdated
| // TODO: right now tasks are still "workspaces" under the hood. | ||
| // We should update this once we have a proper task model. | ||
| ws, err := namedWorkspace(ctx, client, id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this lookup necessary, is it mainly to support e.g. workspace name rather than plain uuid lookups?
We should perhaps consider supporting name lookup in the get task endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah this is basically for convenience due to the current task <-> workspace relationship
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| var ( | ||
| ctx = testutil.Context(t, testutil.WaitShort) | ||
| now = time.Now().UTC() // TODO: replace with quartz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: this is unfortunately flake-prone unless we just test the JSON output which omits the relative time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think switching to the table approach was a good call, nice. A few suggestions pertaining to JSON output but otherwise LGTM.
cli/exp_task_status.go Outdated
| Handler: func(i *serpent.Invocation) error { | ||
| ctx := i.Context() | ||
| ec := codersdk.NewExperimentalClient(client) | ||
| id := i.Args[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just realized this could support multiple args (tasks) as well, and watching each. But let's leave that for future.
Uh oh!
There was an error while loading.Please reload this page.
5baaf27 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closescoder/internal#900
coder exp task statustestutil.MustURLhelperNo robots were harmed in the making of this PR.