Handler to check state.sls errors #19
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new handler to check state.sls errors by implementing a SaltFullJsonReturnHandler that can parse the full JSON output from Salt state.sls calls, enabling proper error detection instead of always marking these calls as successful.
Key changes:
- Implements SaltFullJsonReturnHandler for parsing state.sls JSON responses with proper error handling
- Adds comprehensive test coverage for the new handler including edge cases
- Configures state.sls to use the new handler in the default returners configuration
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| src/main/java/org/rundeck/plugin/salt/output/SaltFullJsonReturnHandler.java | New handler implementation that recursively parses JSON to extract result, comment fields and determine exit codes |
| src/test/java/org/rundeck/plugin/salt/output/SaltFullJsonReturnHandlerTest.java | Comprehensive test suite covering normal operation and error scenarios |
| src/main/resources/defaultReturners.yaml | Configuration update to map state.sls to the new fullJsonCommandParser |
| README.md | Documentation update adding state.sls as an example function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| } | ||
|
|
||
| private List getValues(Object object, String attribute) { |
There was a problem hiding this comment.
The method should use generic types. Change List to List<String> to match the return type and improve type safety.
| private List getValues(Object object, String attribute) { | |
| private List<String> getValues(Object object, String attribute) { |
| public SaltReturnResponse extractResponse(String rawResponse) throws SaltReturnResponseParseException { | ||
| try { | ||
| Gson gson = new Gson(); | ||
| Object result = gson.fromJson(rawResponse, Object.class); |
There was a problem hiding this comment.
Inconsistent indentation. The line uses tabs instead of spaces, which is inconsistent with the surrounding code formatting.
| Object result = gson.fromJson(rawResponse, Object.class); | |
| Object result = gson.fromJson(rawResponse, Object.class); |
| List<String> results=getValues(result, exitCodeKey); | ||
| if (results.size() == 0) { | ||
| throw new SaltReturnResponseParseException("No " + exitCodeKey + " attribute in JSON output"); | ||
| } | ||
| Integer exitCode=new Integer(0); | ||
| for (String tmp : results) { | ||
| if (!"true".equalsIgnoreCase(tmp)) | ||
| exitCode=1; | ||
| } | ||
| response.setExitCode(exitCode); |
There was a problem hiding this comment.
Inconsistent indentation throughout this block. The code mixes tabs and spaces, and some lines have incorrect indentation levels. All indentation should be consistent with the project's formatting standards.
| if (results.size() == 0) { | ||
| throw new SaltReturnResponseParseException("No " + exitCodeKey + " attribute in JSON output"); | ||
| } | ||
| Integer exitCode=new Integer(0); |
There was a problem hiding this comment.
Use Integer.valueOf(0) instead of new Integer(0). The constructor is deprecated and creates unnecessary object instances.
| Integer exitCode=new Integer(0); | |
| Integer exitCode=Integer.valueOf(0); |
|
|
||
| if (exitCodeKey != null) { | ||
| List<String> results=getValues(result, exitCodeKey); | ||
| if (results.size() == 0) { |
There was a problem hiding this comment.
Use results.isEmpty() instead of results.size() == 0 for better readability and performance.
|
|
||
| String result = "true"; | ||
| String comment = "some comment"; | ||
| String json = String.format(SAMPLE_JSON_TEMPLATE, comment, result, comment, result); |
There was a problem hiding this comment.
The String.format call has 4 arguments but the template only has 2 placeholders (%s). This will cause an IllegalFormatException at runtime.
|
|
||
| String result = "true"; | ||
| String comment = "some comment"; | ||
| String json = String.format(SAMPLE_JSON_TEMPLATE, comment, result, comment, result); |
There was a problem hiding this comment.
All String.format calls have the same issue - 4 arguments provided but only 2 placeholders in the template. This will cause IllegalFormatException at runtime.
|
|
||
| String result = "true"; | ||
| String comment = "some comment"; | ||
| String json = String.format(SAMPLE_JSON_TEMPLATE, comment, result, comment, result); |
There was a problem hiding this comment.
All String.format calls have the same issue - 4 arguments provided but only 2 placeholders in the template. This will cause IllegalFormatException at runtime.
|
|
||
| String result = "true"; | ||
| String comment = "some comment"; | ||
| String json = String.format(SAMPLE_JSON_TEMPLATE, comment, result, comment, result); |
There was a problem hiding this comment.
All String.format calls have the same issue - 4 arguments provided but only 2 placeholders in the template. This will cause IllegalFormatException at runtime.
|
|
||
| String result = "true"; | ||
| String comment = "some comment"; | ||
| String json = String.format(SAMPLE_JSON_TEMPLATE, comment, result, comment, result); |
There was a problem hiding this comment.
All String.format calls have the same issue - 4 arguments provided but only 2 placeholders in the template. This will cause IllegalFormatException at runtime.
1 participant
Hello,
It's not possible to change the output for a call via the salt-api to have the same output as the cmd.run_all command (and use SaltJsonReturnHandler). If it was possible I would write a new salt-api outputter.
I want to check errors, so I can't associate "alwaysSuccessful" to state.sls.
I write a new handler to parse the output from a statle.sls call.
Thanks,
Rémy