Skip to content
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

Updated Get-AzAutomationJobOutputRecord to handle JSON and Text record values #8607

Closed
wants to merge 3 commits into from

Conversation

lwajswaj
Copy link
Contributor

Description

This PR addresses issue #7977 and #8600. Root cause of the issue was identified as stream outputs always expecting to be a JSON file; however, this ain't true in all cases as String or PSObjects are not returned as JSON. To fix it, a RegEx was put in place to identify JSON output from text output.

Checklist

@cormacpayne
Copy link
Member

@lwajswaj Hey Leandro, thanks for another contribution 😀

@vrdmr would you mind taking a look at these changes. or assigning this to someone on our side to take a look?

@lwajswaj
Copy link
Contributor Author

lwajswaj commented Mar 1, 2019

Any update on this PR?

@cormacpayne
Copy link
Member

@lwajswaj the proper contacts from the Automation team have been notified offline -- @NicoletazMS @NarayanThiru

@safeermohammed
Copy link
Contributor

@lwajswaj, Do we have any tests?

@lwajswaj
Copy link
Contributor Author

lwajswaj commented Mar 9, 2019

@safeermohammed, this should be covered by existing (if any) test cases.

@cormacpayne, any update on this review?

@vladimir-shcherbakov
Copy link
Contributor

@NicoletazMS @NarayanThiru @vrdmr Would you mind taking a look at these changes?

@lwajswaj
Copy link
Contributor Author

@vladimir-shcherbakov, @NicoletazMS, @NarayanThiru , @vrdmr , any update on this?

@NarayanThiru
Copy link
Member

Leandro, can you please explain how you tested the fix? Asking because, when I look at the existing code, it is already taking care of the case where the jobStream.Value may not be JSON. The Deserialize (line 61) would throw exception if the value is not JSON format; and the catch block checks if the exception message contains 'Invalid JSON primitive'. If it doesn't it assigns the value to paramValue (line 68). Was this not working? Moreover, the fix will match the pattern {"error" : {"code" : "AuthenticationFailed", "message" : "Authentication failed."} }; but not the pattern {"error" : {"code" : "AuthenticationFailed", "message" : "Authentication failed."}, {"value" : "somevalue" } }. Will there always be only one child property? I am new to this code base - so pardon my questions; and apologize for the delay in responding.

@vladimir-shcherbakov
Copy link
Contributor

@lwajswaj Ping on the comments above.

@MiYanni
Copy link
Contributor

MiYanni commented Mar 28, 2019

@lwajswaj No activity for 2 weeks. Closing. Feel free to reopen the PR when you have time to address the comments above.

@MiYanni MiYanni closed this Mar 28, 2019
@lwajswaj
Copy link
Contributor Author

Hello, I've been with some business trips + vacations and it was not possible to stay on top of this PR. How should I proceed? opening a new one or can this one be re-open?

@NarayanThiru, in order to test it, just reference to the linked issues. If you try to repro, you will see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants