feat: Add diagnostics parameter

[SanderNugteren]

Description

I added the diagnostics parameter for Log Analytics. However, I was unable to fully test the changes due to issues with the local setup. I would appreciate it if someone with a working environment could verify and complete the testing.

Fixes #3519
-->

Pipeline Reference

Pipeline

Type of Change

• Update to CI Environment or utilities (Non-module affecting changes)
• Azure Verified Module updates:
  • Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
    • Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
    • The bug was found by the module author, and no one has opened an issue to report it yet.
  • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
  • Breaking changes and I have bumped the MAJOR version in version.json.
  • Update to documentation

Checklist

• I'm sure there are no other open Pull Requests for the same update/change
• I have run Set-AVMModule locally to generate the supporting module files.
• My corresponding pipelines / checks run clean and green without any errors or warnings

[SanderNugteren]

@SanderNugteren please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@microsoft-github-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Waternet"

[AlexanderSehr]

Hey @SanderNugteren,
happy to do so by rebasing your PR (which keeps you as the author) and testing it in my environment. However, before doing so, I'd need to ask you to merge the latest upstream AVM version into your fork and resolve the conflicts. It appears your version of the module is missing some updates that were already implemented in the upstream repository.
Just make sure your actions are disabled before pulling the changes into your fork as this may result in all of them being triggered (if not disabled).

[SanderNugteren]

Hi @AlexanderSehr! I resolved the conflicts. Thanks for the help in getting this merged!

[AlexanderSehr]

Does it make sense that logAnalytics is optional?
E.g., because just setting an empty diagnostics object already configures something.

If not, it may make sense to either make it mandatory, or even remove one level of nesting by changin it to

diagnostics: !empty(diagnostics) ? {
  logAnalytics: {
    logType: diagnostics!.logType
    workspaceId: diagnostics!.workspaceId
    workspaceKey: diagnostics!.workspaceKey
    workspaceResourceId: diagnostics!.?workspaceResourceId
    metadata: diagnostics!.?metadata
  }
} : null

[SanderNugteren]

I was also thinking that, but I thought that in the future the diagnostics might contain more fields than just the logAnalytics field (e.g. having different diagnostics implementations). In that case it might change which diagnostics field you choose to use and they should allbe optional.

But perhaps this is over-engineering for a possible future situation 😉. If you think it's not necessary to account for this, then I think implementing your suggestion of removing the nesting would be best. What do you think?

[AlexanderSehr]

Either is fine for me, and ultimately it might be best if the module owner @JPEasier makes this decision.

You input is much appreciated and I share your concern. 💪
That being said, there's also a best practice in engineering to not plan for possible future scenarios that may never happen 😄 I guess if it does happen it could still be changed alongside a major version change to tell users as much - and transitioning from one model to the other would be as easy as to adding this intermediate level. Either way - it's not outrageous.

One element would help the decision though - and that is to find out if even a deployment like diagnostics: {} (which you can currenlty do as logAnalytics is optional does any thing. A good example for this are networkAcls which usually, even if empty, enable a service's firewall.
If this is the case here to, I'd strongly recommend to keep your current implementation. If not - then it remains up for debate an personally I'd lean towards my suggestion. But I'm biased, so ... 😄

[SanderNugteren]

Hi @AlexanderSehr ! I don't see an answer from @JPEasier yet. How should we proceed with this PR?

[AlexanderSehr]

Hey @SanderNugteren, I just reached out to the owner internally. The notification via GitHub may have gotten lost.

[JPEasier]

Hey @SanderNugteren,
I understand it like this, diagnostics: {} has only one parameter: logAnalytic . in this case i would suggest to set logAnalytic as mendetory, then the conversion is quite simple if more parameters are added.

So exactly what @AlexanderSehr wrote in his code suggestion :)

[JPEasier]

Thank you very much for your contribution!
I see some conflicts, please review and update your branch.

[JPEasier]

as disscussed, lets make the logAnalytics mandatory. :)