Adding support to osctrl-cli to look up nodes via osctrl-api#658
Merged
Adding support to osctrl-cli to look up nodes via osctrl-api#658
osctrl-cli to look up nodes via osctrl-api#658Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds support for looking up nodes via the osctrl API in the CLI and cleans up related types and handlers.
- Removes the old
ApiLookupResponsetype and swaps schema references to useOsqueryNode - Implements a new
lookupCLI command and the underlyingOsctrlAPI.LookupNodemethod - Refactors handler code to return raw
OsqueryNode, and standardizes some error messages
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/types/types.go | Deleted unused ApiLookupResponse type |
| osctrl-api.yaml | Changed lookup response reference to OsqueryNode schema |
| cmd/cli/node.go | Refactored showNode, added lookupNode using _showNode helper |
| cmd/cli/main.go | Added lookup command; lowercased backend creation error messages |
| cmd/cli/api-node.go | Added LookupNode implementation on OsctrlAPI |
| cmd/api/handlers/utils.go | Removed unused node-to-API conversion helper |
| cmd/api/handlers/nodes.go | Updated handler to return nodes.OsqueryNode directly |
Comments suppressed due to low confidence (5)
cmd/cli/api-node.go:70
- [nitpick] Variable name 'l' is ambiguous (looks like '1'); consider renaming to 'req' or 'lookupReq' for clarity.
l := types.ApiLookupRequest{
cmd/cli/main.go:1704
- [nitpick] Error message casing is inconsistent with other messages; consider standardizing (e.g., start with uppercase or lowercase consistently).
return fmt.Errorf("failed to create backend - %w", err)
cmd/cli/main.go:1709
- [nitpick] Error message casing is inconsistent with other messages; consider standardizing (e.g., start with uppercase or lowercase consistently).
return fmt.Errorf("failed to create backend - %w", err)
cmd/cli/node.go:255
- New lookup functionality is introduced without accompanying unit or integration tests; consider adding tests for
lookupNodebehavior.
func lookupNode(c *cli.Context) error {
cmd/cli/main.go:1210
- [nitpick] The alias 'f' for the 'lookup' command may be non-intuitive; consider using a more descriptive alias like 'l' or 'id'.
Aliases: []string{"f"},
Use `bytes.NewReader(jsonMessage)` instead of converting to string to avoid unnecessary memory allocation. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Constructing the URL using `path.Join` to avoid manual concatenation errors. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
osctrl-clito look up nodes viaosctrl-api, merged in Lookup node by identifier inosctrl-api#657lookupcommand