Azure-Sentinel icon indicating copy to clipboard operation
Azure-Sentinel copied to clipboard

Published 20 hours ago • verified publisher icon Azure

Fixed 'jsonData' parameters and removed escape characters

Open TheCloudScout opened this issue 2 years ago • 14 comments

Required items, please complete

Change(s):

  • Updated workbook defintion for DataCollectionHealthMonitoring.json

Reason for Change(s):

  • Json had incorrect parameters 'jsonData' which caused deployments from code to fail. Improper serialization. These improperly defined object will remain even if you export it again for deployment from an automated pipeline for example. I tested out this updated version which I now can properly import and export as much as I want.

Version updated:

  • Yes

Testing Completed:

  • Yes

Checked that the validations are passing and have addressed any issues that are present:

  • See guidance below

TheCloudScout avatar Dec 07 '22 15:12 TheCloudScout

Hi @TheCloudScout, We will review the changes and provide feedback. Thanks

v-mchatla avatar Dec 09 '22 05:12 v-mchatla

Hi @TheCloudScout, I'm getting below error while trying to test the workbook image whereas it's working as expected from the previous version of the workbook image

v-mchatla avatar Dec 13 '22 11:12 v-mchatla

Hi @TheCloudScout, Can you please suggest if Im missing something here. Thanks

v-mchatla avatar Dec 15 '22 05:12 v-mchatla

Hi @TheCloudScout, Can you please suggest if Im missing something here. Thanks

I'm sorry for the late reply. I checked my changes and found the error. Right after I've created the pull request, I thought that I needed to update its version as mentioned in the guidelines.

On line 2 I changed:

"version": "Notebook/1.0", and "version": "KqlParameterItem/1.0",

into:

"version": "Notebook/1.1", and "version": "KqlParameterItem/1.1",

Which broke the template. So I've reverted this back.

Please try again, you'll notice that it works again.

The reason for this change (jsonbody) was mainly because we couldn't deploy the template as an ARM template from an automated pipeline. The Azure DevOps agents breaks on an error because of the json escaping within the jsonbody. By expanding this and removing the escape characters, it works flawlessly now on my end. I haven't made any changes to other parts of this workbook template.

TheCloudScout avatar Dec 16 '22 12:12 TheCloudScout

Hi @TheCloudScout, Thanks for the updated workbook. I will review and share the feedback Thanks

v-mchatla avatar Dec 21 '22 05:12 v-mchatla

Hi @TheCloudScout, I'm occupied with priority work. I will review and share the feedback soon. Thanks

v-mchatla avatar Dec 23 '22 04:12 v-mchatla

Hi @TheCloudScout, I'm occupied with priority work. I will review and share the feedback today. Thanks

v-mchatla avatar Dec 28 '22 04:12 v-mchatla

Hi @TheCloudScout, Thanks for the quick fix. Now I'm able to view the dashboard properly but still there are few queries were failing. Can you please check and confirm from your end. image image

image

Thanks

v-mchatla avatar Dec 29 '22 10:12 v-mchatla

Thanks @v-mchatla for you thorough testing! And you're very right! I completely missed the fact that these help functions stopped working, I was only focussen on the graphs and lists.

The main reason why I wanted to correct/update this template is because it was giving me issues when deploying it as an ARM template from an automated pipeline. Apparently the jsonData is required to remain a string in a single line, instead of expanded into actual json rows. I went back to my initial issue and fixed it by reverting the jsonData lines back to its original state, and putting it between a string() function in my ARM template:

"jsonData": "[string('[{ \"value\": \"Yes\", \"label\": \"Yes\"},\r\n {\"value\": \"No\", \"label\": \"No\", \"selected\":true }]')]"

Because we both spend quite some effort in this pull request, let's make sure it wasn't for nothing! ;-) I still was able to correct two very minor things:

  • I found a placeholder/leftover subscriptionId which should be removed I guess
  • Fixed indentation on the last few lines.

Let me know what you think.

TheCloudScout avatar Dec 30 '22 11:12 TheCloudScout

Hi @TheCloudScout, Thanks for the detailed explanation. I will try to review and share my thoughts. Thanks

v-mchatla avatar Jan 04 '23 05:01 v-mchatla

Hi @TheCloudScout, Sorry, I didnt get a chance to work on this from last 2days, I will allocate sometime today for this and share you the update. Thanks

v-mchatla avatar Jan 06 '23 05:01 v-mchatla

Hi @TheCloudScout, Please expect some delay as we are occupied with priority work. Thanks

v-mchatla avatar Jan 11 '23 05:01 v-mchatla

Hi @TheCloudScout, Can you please fix the version details, so that I can merge the PR. Thanks

v-mchatla avatar Jan 13 '23 03:01 v-mchatla

Changes look good. Need one small change - please increment the version in workbook metadata file. Currently its 1.0.0 update it to 1.0.1

Sorry wasn't aware of this metadata file. I've updated the version.

Thanks for all of your efforts and feedback on this change!

TheCloudScout avatar Jan 13 '23 11:01 TheCloudScout

Hi @TheCloudScout, please try pushing this branch again by taking latest from master since some checks have not been completed. Thanks.

v-atulyadav avatar Jan 18 '23 04:01 v-atulyadav

@TheCloudScout, changes looks good, just update the branch once from master for the checks to run. Thanks.

v-sabiraj avatar Jan 18 '23 05:01 v-sabiraj

Hi @TheCloudScout, Can you try taking latest from master, checks need to be completed in order to proceed with approval. Thanks

v-mchatla avatar Jan 20 '23 05:01 v-mchatla

Hi @v-sabiraj, @v-mchatla, @v-atulyadav,

I've made a slight change by compacting the WorkbooksMetadata.json to trigger a re-run on all tests. And this appeared to be working since all tests are currently running.

TheCloudScout avatar Jan 20 '23 15:01 TheCloudScout