-
Notifications
You must be signed in to change notification settings - Fork 44
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kcctl 17 #77
Kcctl 17 #77
Conversation
…operties for basic auth
* Showing more meaningful error message * Avoiding Commons Lang dependency * Updating versions
@tonyfosterdev, this supersedes your original PR #68. You can push further commits to this PR, if you like. |
@tonyfosterdev, so I was doing some research about the error handling question last night; there's means of error handling in PicoCLI, but I'm not sure whether we can enable this on a global level when embedding PicoCLI via Quarkus. We may have to register the error handler for each command (i.e. the auth failure handling would be implemented once, but there'd still be a change required to all the commands for registering that handler). Not ideal, but ok, I suppose. WDYT? |
@tonyfosterdev, just learned there actually is a way, customizing the command line object: https://quarkus.io/guides/picocli#customizing-picocli-commandline-instance. This should let us set the error handler in one single place indeed. |
@gunnarmorling I didn't have permission to push to this branch, so I reforked, and pushed a new PR. Apologies for the mess here! I used the method you outlined above. I'm very new to Java, so making my way through the dependency injection took me a little bit, but it turned out to be really easy in the end. I also added unit tests for handler. |
Oh, wow, I would not have thought that at all. Really great work!
Excellent! Testing is done sparsely so far (we'll need some Testcontainers set-up to test the actual commands), so it's great to see some improvements in that area. Going to close this one in favour of #79. |
No description provided.