feat: Improve command line argument handling and user experience #188
Conversation
tomaz-lc
changed the title
tomaz-lc
force-pushed
the
feat/better-cli-arguments-handling-new-flags
branch
11 times, most recently
from
024f277 to
a3dc2c8
Compare
…andling in various error / edge conditions and introduce new "--help" and "--version" flag. NOTE: This is just a quick PoC / proposal so we can brainstorm better solution. Ideal better command line argument handling would require larger refactor.
…inary end to end tests and corresponding GHA workflow which runs unit tests with coverage + end to end tests.
…e mode and other edge cases and refactor the code a bit more.
add lint / format step to the CI workflow.
tomaz-lc
force-pushed
the
feat/better-cli-arguments-handling-new-flags
branch
from
1094d6d to
84d2888
Compare
| "github.com/refractionPOINT/usp-adapters/evtx" | ||
| "github.com/refractionPOINT/usp-adapters/file" | ||
| "github.com/refractionPOINT/usp-adapters/gcs" | ||
| "github.com/refractionPOINT/usp-adapters/hubspot" |
There was a problem hiding this comment.
This fixes a minor lint / go fmt issue which managed to sneak into master.
scenarios: - Multiple adapter configs specified in a single YAML config file - Config options are specified using CLI arguments - Config options are specified using environment variables (currently broken) Also fix tests and add missing status assertions for bats "run" statements.
which is not needed to avoid printing usage multiple times.
|
I added more test cases to try to cover all the documented scenarios / possible uses of the tool:
It turns out that the last approach doesn't appear to be working. There appear to be multiple reasons for that and code would need multiple adjustments to make it work (possibly also handling special characters in environment variable names such as ".", etc), but the main one appears to be this line - usp-adapters/containers/general/tool.go Line 359 in 3456a8c For the time being (to keep the scope down and until we reach a consensus on how to approach it), I decided not to fix it. |
| func parseConfigsFromFile(filePath string) ([]*GeneralConfigs, error) { | ||
| f, err := os.Open(filePath) | ||
| if err != nil { | ||
| printUsage() |
There was a problem hiding this comment.
This was removed since the tool previously printed usage string multiple times (duplication) during some failure scenarios.
| } | ||
| b, err := io.ReadAll(f) | ||
| if err != nil { | ||
| printUsage() |
There was a problem hiding this comment.
This was removed since the tool previously printed usage string multiple times (duplication) during some failure scenarios.
| } | ||
|
|
||
| if jsonErr != nil && yamlErr != nil { | ||
| printUsage() |
There was a problem hiding this comment.
This was removed since the tool previously printed usage string multiple times (duplication) during some failure scenarios.
the GHA workflow. For now, only report and fail on new issues due to multiple existing issues being presents (those will be addressed separately). Also split workflow into two jobs: - lint jobs -> runs lint + static analysis using a single Go version - tests -> runs unit and integration tests under multiple Go versions
tomaz-lc
force-pushed
the
feat/better-cli-arguments-handling-new-flags
branch
from
e688883 to
b84e12b
Compare
with known data race (due to possible race in tail package) with race detection enabled.
| @@ -1,3 +1,6 @@ | |||
| //go:build skiptests | |||
| // +build skiptests | |||
There was a problem hiding this comment.
There is one known race condition in tail library + how we use it (we call tell before stop).
Description
This pull request is an attempt to start a discussion about improving command line argument handling in various normal and edge conditions and improving the end user experience.
Additionally, it also introduces the new
-helpand-versionCLI flag.Background, Context
While using the tool, I noticed it includes sub-optional user experience in various scenarios, including:
Examples
Before
After
Proposed Implementation
In this PR I propose a simple change to handle some (but not yet all) of those edge cases better. It's by no means a final solution or anything like that.
I just wanted to start a discussion on how to handle that better and once we reach a consensus (e.g. should we use
flagpackage, should we use go style -flags or --flags, should I just fix main issue or fully refactor the CLI handling - without extensive end to end test coverage, full refactoring will be risky, etc), I can implement those changes.It's also worth pointing out that currently the code does some unconventional thing with args (e.g. service mode and handling any arg starting with
-as pass through to service mode) which means we could not use flag package directly without more boiler plate. This means it could defeat the purpose and just increase the complexity so the current simple and naive implementation is better.Additionally, I also included some simple bats (https://github.com/bats-core/bats-core) based end to end tests. If people have preference for some other end to end testing framework (e.g. some Golang based one such as https://github.com/elastic/testcli), we can discuss that as well.
TODO
Related issues
Fix refractionPOINT/tracking#2983