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 #9031

Merged
merged 9 commits into from
May 6, 2019

Conversation

lwajswaj
Copy link
Contributor

This is a re-open of PR 8607.

Answering @NarayanThiru comments: Current logic doesn't work as the error message and code is different (regardless that it's easier and cleaner to check on the output format rather than attempt and failing). Regarding the issue and how to repro/test, refer to the linked issues they contain a lot of good information.

CC: @vladimir-shcherbakov, @MiYanni

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

@markcowl markcowl added Service Attention This issue is responsible by Azure service team. Automation labels Apr 22, 2019
@markcowl
Copy link
Member

@vrdmr @jemex Can one of you review this PR?

@NarayanThiru
Copy link
Member

The changes look good. @thomasyip-msft, @vrdmr, @safeermohammed, can any of you merge this PR?

src/Automation/Automation/ChangeLog.md Outdated Show resolved Hide resolved
@markcowl markcowl added needs-revision and removed needs-review Service Attention This issue is responsible by Azure service team. labels May 2, 2019
@markcowl markcowl assigned lwajswaj and unassigned vrdmr and jemex May 2, 2019
@markcowl
Copy link
Member

markcowl commented May 2, 2019

@lwajswaj if you can make the changelog change, we can fix this. Is there a simple way we can add test automation for this fix, to prevent future regressions?

@lwajswaj
Copy link
Contributor Author

lwajswaj commented May 2, 2019

@lwajswaj if you can make the changelog change, we can fix this. Is there a simple way we can add test automation for this fix, to prevent future regressions?

I might be able to add a quick test under the scenario tests. Kind of like starting the mock up runbook and retrieving its output stream (that's the current error condition)

@lwajswaj
Copy link
Contributor Author

lwajswaj commented May 5, 2019

@lwajswaj if you can make the changelog change, we can fix this. Is there a simple way we can add test automation for this fix, to prevent future regressions?

@markcowl, Just added tests for this scenario...... along the way, I found the tests for runbook execution were not executing properly, for example expected result was never evaluated, that's why I included a couple of extra changes:

  • Renamed fastjob.ps1 to Test-PowershellRunbook.ps1 (name is meaningful now) also migrated this script from using workflow to plain powershell. Added additional outputs since that's the error condition for 'Get-AzAutomationJobOutputRecord' otherwise test was successful with a single integer output.
  • Renamed fastjob.py to TestPythonRunbook.py (name is meaningful too). Also added additional outputs (same reason as before)
  • I found 'Test-RunbookWithParameter' was not using its parameter $expectedResult, I changed it to include this assertion as part of the test

Of course, added the tests for 'Get-AzAutomationJobOutputRecord' for preventing regressions.

@markcowl
Copy link
Member

markcowl commented May 6, 2019

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

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.

6 participants