Skip to content

trim trailing null from callout script get-attributes output#85

Merged
jonner merged 1 commit into
mdevctl:masterfrom
collinwalling:trim-null-from-get-attrs-output
Apr 5, 2023
Merged

trim trailing null from callout script get-attributes output#85
jonner merged 1 commit into
mdevctl:masterfrom
collinwalling:trim-null-from-get-attrs-output

Conversation

@collinwalling

@collinwalling collinwalling commented Apr 5, 2023

Copy link
Copy Markdown
Contributor

A trailing null will make serde_json::from_str fail. Let's trim the string if a null is present to make it happy.

Fixes #84

Note, this will create a merge conflict in PR #83. If this change is acceptable, I can either rebase Boris' patches on top of this, or I can fit this patch in nicely after the conflicting commit.

@collinwalling collinwalling force-pushed the trim-null-from-get-attrs-output branch from 53b2823 to 5b7e6f5 Compare April 5, 2023 20:54
@jonner

jonner commented Apr 5, 2023

Copy link
Copy Markdown
Member

Consider adding a test while we're at it. For example, something like this could be squashed into your patch:

diff --git a/src/tests.rs b/src/tests.rs
index 215ff2a..a98b5c5 100644
--- a/src/tests.rs
+++ b/src/tests.rs
@@ -1959,6 +1959,16 @@ fn test_callouts() {
             test.populate_callout_script("good-json.sh");
         },
     );
+    test_get_callout(
+        "test_callout_good_json_null",
+        Expect::Pass,
+        Uuid::parse_str(DEFAULT_UUID).unwrap(),
+        DEFAULT_PARENT,
+        DEFAULT_TYPE,
+        |test| {
+            test.populate_callout_script("good-json-null-terminated.sh");
+        },
+    );
     test_get_callout(
         "test_callout_bad_json",
         Expect::Fail(None),
diff --git a/tests/callouts/good-json-null-terminated.sh b/tests/callouts/good-json-null-terminated.sh
new file mode 100755
index 0000000..c5e12d7
--- /dev/null
+++ b/tests/callouts/good-json-null-terminated.sh
@@ -0,0 +1,2 @@
+#!/bin/sh
+echo -ne "[{\"attribute0\": \"VALUE\"}]\0"

@collinwalling

Copy link
Copy Markdown
Contributor Author

Fair point. Thanks for providing this.

@collinwalling

Copy link
Copy Markdown
Contributor Author

Would you like for me to add your sign-off to the patch since you provided the test case?

@jonner

jonner commented Apr 5, 2023

Copy link
Copy Markdown
Member

I suppose that would probably be more proper, why not.

@jonner

jonner commented Apr 5, 2023

Copy link
Copy Markdown
Member

By the way, I notice that your commit message says "Addresses #84". If you change that to one of the phrases mentioned here (basically: 'closes', 'fixes', or 'resolves'), github will autoclose the issue when we merge this.

A trailing null will make serde_json::from_str fail. Let's trim the
string if a null is present to make it happy.

Fixes mdevctl#84

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
@collinwalling collinwalling force-pushed the trim-null-from-get-attrs-output branch from 5b7e6f5 to f5f7694 Compare April 5, 2023 21:57
@collinwalling

Copy link
Copy Markdown
Contributor Author

Fixed up. Thanks @jonner

@jonner

jonner commented Apr 5, 2023

Copy link
Copy Markdown
Member

Let's just merge this and rebase #83 if necessary. That one is going to take more time to review.

@collinwalling

Copy link
Copy Markdown
Contributor Author

Sure. Boris is out-of-office for a while. Any suggestions on how I can handle rebasing his branch? Is it as simple as forking his changes, rebasing, then creating a new PR? /shrug.

@jonner

jonner commented Apr 5, 2023

Copy link
Copy Markdown
Member

I think I might have the ability (as maintainer) to edit his PR branch. So maybe I could rebase it and force push. Otherwise we can figure it out later ;)

@jonner jonner merged commit 60e3777 into mdevctl:master Apr 5, 2023
@collinwalling collinwalling deleted the trim-null-from-get-attrs-output branch April 5, 2023 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trailing NULL causes serde_json to fail

2 participants