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.

Sign up for GitHub

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

Jump to bottom

Added the api command. #1946

Merged
merged 35 commits into from
Apr 7, 2022
Merged

Added the api command. #1946

merged 35 commits into from
Apr 7, 2022

Conversation

ericpromislow
Copy link
Contributor

@ericpromislow ericpromislow commented Mar 31, 2022 •
edited
Loading

Fixes #1903

Currently supports these options:

--method | -X method (default PUT when input is specified, GET otherwise)
--input ( FILE | - ) (like the current settings.json file, `-` for stdin)
--body | -b input-body, normally JSON

And these endpoints:

GET settings
PUT settings <BODY>
PUT shutdown

Signed-off-by: Eric Promislow [email protected]

@ericpromislow ericpromislow requested a review from mook-as March 31, 2022 21:40
@jandubois
Copy link
Member

I expected this to work:

$ ./rdctl api list-settings
Error: server error return-code 404: 404 Not Found
Usage:
  rdctl api [flags]
[...]

Also the usage just says rdctl api [flags]. It should also mention the query-path argument.

Specifying an explicit API version works:

$ ./rdctl api /v0/list-settings
Status: {
  "version": 4,
[...]
}.

But it prefixes the output with the Status: string and appends a spurious . at the end, breaking the JSON format:

$ ./rdctl api /v0/list-settings | jq .version
parse error: Invalid numeric literal at line 1, column 7
$ ./rdctl api /v0/list-settings | sed 's/^Status: //' | sed 's/}\.$/}/' | jq .version
4

@ericpromislow ericpromislow self-assigned this Mar 31, 2022
@ericpromislow ericpromislow added this to the Next milestone Mar 31, 2022
@jandubois jandubois added kind/enhancement New feature or request kind/quality quality improvements, refactoring, Automation via CI, E2E, Integration, CLI or REST API labels Mar 31, 2022
@ericpromislow ericpromislow marked this pull request as draft March 31, 2022 22:53
mook-as
mook-as requested changes Mar 31, 2022
View reviewed changes
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Please run npm run lint — the go files are not correctly formatted (and our CI isn't catching that correctly).

(Abandoning review because the PR was marked as draft)

@jandubois jandubois removed this from the Next milestone Apr 1, 2022
@ericpromislow ericpromislow force-pushed the 1903-add-api-command branch from 5fc0462 to 7a299f2 Compare April 1, 2022 22:19
@ericpromislow ericpromislow marked this pull request as ready for review April 4, 2022 21:15
@ericpromislow ericpromislow force-pushed the 1903-add-api-command branch from 138538c to 72b2b68 Compare April 4, 2022 21:21
@ericpromislow ericpromislow requested a review from mook-as April 4, 2022 21:25
@ericpromislow ericpromislow mentioned this pull request Apr 4, 2022
Closed
mook-as
mook-as requested changes Apr 5, 2022
View reviewed changes
@ericpromislow ericpromislow mentioned this pull request Apr 5, 2022
Merged
@ericpromislow ericpromislow force-pushed the 1903-add-api-command branch 2 times, most recently from 66c3f4d to a8bf0a5 Compare April 5, 2022 19:04
mook-as
mook-as requested changes Apr 5, 2022
View reviewed changes
@jandubois
Copy link
Member

This is wrong:

$ resources/darwin/bin/rdctl api foo; echo $?
{"error":{"message":"404 Not Found","documentation_url":null}}
Unknown command: GET /v0/foo
0

The exit code should be non-zero when the API call failed. And there is no reason to wrap the error properties with another object, this should output:

$ resources/darwin/bin/rdctl api foo; echo $?
{"message":"404 Not Found","documentation_url":null}
Unknown command: GET /v0/foo
1

The HTTP status code (and exit code from rdctl) tell you that the output is an error object, and not the expected data. The error property is not some kind of reserved word to indicate a failure, so the wrapping isn't necessary and just makes extracting the message more complex than it needs to be.

mook-as
mook-as requested changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

In general, if we're looping over test parameters we should have subtests.

ericpromislow and others added 6 commits April 6, 2022 09:50
@ericpromislow
Added the api command.
a464bd3 
Currently supports these options:
-X method (default PUT when input is specified, GET otherwise)
--input FILE (like the current settings.json file)
--raw-field|-f NAME=VALUE - string VALUEs must be double-quoted
--field|-F NAME=VALUE - double quotes added where needed

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Add happy-path e2e tests for rdctl.
d89f70c 
- Start with list-settings and set subcommands.

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Add more integration tests for rdctl.
cdaa5f9 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Make error output from command-server more consistent.
29019b4 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
More work on rdctl api:
9739506 
- Change /list-settings and /set endpoints to /settings
- Remove the --field and --rawField options until we have parameters

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Add unit tests against rdctl.
b156f0b 
Signed-off-by: Eric Promislow <[email protected]>
Eric Promislow and others added 7 commits April 6, 2022 09:50
@ericpromislow
Fix a whitespace fault.
7b09c99 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
rdctl api "" -> error
99fb239 
Also tabify the go code.

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Add a test to check for both --body and --input
062e99f 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Add an optional error field when rdctl fails to run.
b6df694 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Put tests into subtests by category.
f097e6e 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Wrap error packets in an error field.
ce88613 
Makes it easier to determine which output from an `rdctl api`
call is a server error.

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Don't wrap server errors in an error: object.
7dec9e2 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow ericpromislow force-pushed the 1903-add-api-command branch from 6ab5271 to 7dec9e2 Compare April 6, 2022 17:03
@ericpromislow ericpromislow force-pushed the 1903-add-api-command branch from 2615da3 to 2d45a4e Compare April 6, 2022 23:59
mook-as
mook-as requested changes Apr 7, 2022
View reviewed changes
@ericpromislow
Revise e2e tests for rdctl.
506c3b8 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Reorganize tests
e8b467f 
- Use nested categories.
- Use nested tests for looping over options with feedback.

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Improve test organization.
6ca56f5 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow ericpromislow force-pushed the 1903-add-api-command branch from ec188c3 to 6ca56f5 Compare April 7, 2022 18:09
@ericpromislow
Rearrange imports.
abc518b 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow ericpromislow requested review from mook-as April 7, 2022 18:30
@ericpromislow ericpromislow merged commit d843e25 into main Apr 7, 2022
@ericpromislow ericpromislow deleted the 1903-add-api-command branch April 7, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jandubois jandubois Awaiting requested review from jandubois

@mook-as mook-as Awaiting requested review from mook-as

Assignees

@ericpromislow ericpromislow

Labels
kind/enhancement New feature or request kind/quality quality improvements, refactoring, Automation via CI, E2E, Integration, CLI or REST API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add rdctl api command
3 participants
@ericpromislow @jandubois @mook-as