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

feat: Add the creation of 'Microsoft.Portal/dashboards' in Insights Module - avm/res/insights/component #1974

Closed
wants to merge 12 commits into from

Conversation

zedy-wj
Copy link
Member

@zedy-wj zedy-wj commented May 20, 2024

Description

Fixes #1130

Pipeline Reference

Pipeline
avm.res.insights.component

Type of Change

  • Update to CI Environment or utlities (Non-module effecting changes)
  • [ x] 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

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

@jongio - for notification.

@zedy-wj zedy-wj requested review from a team as code owners May 20, 2024 07:17
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels May 20, 2024
@AlexanderSehr
Copy link
Contributor

Hey @zedy-wj,
thank you for the contribution. From where I'm standing (and considering the AVM specs) this needs some updates. Either

  • The module is updated as is. That means, instead of adding the new resource as a child-module it shoud instead be a nested template in a /modules/ subfolder like here for the VM NIC & backup (as it is no child of Microsoft.Insights/components in the API sense), OR
  • It should be a Pattern module that uses the Insights module as a nested resource and build the dahsboard around it.

What it qualifies for is subject to debate as the spec leaves a bit of wiggle room and it comes down to

  • how closely dashboards & app insights are correlated and
  • how common the use case is to deploy the 2 together

As this is not an easy answer, I'd also as the other maintainers @jtracey93, @eriqua, @ChrisSidebotham and of course the module owner @krbar to share your thoughts.

@jtracey93
Copy link
Contributor

thanks for the PR.

I think this feels like a new pattern module to me. As the dashboards could be a resource module own their own, and should be ideally. Then can be wrapped in many pattern modules

@jongio
Copy link
Member

jongio commented May 21, 2024

  1. dashboard should be independent module
  2. it could be included in a pattern module in the future

for now, let's get the dashboard module created if not there yet

@krbar
Copy link
Contributor

krbar commented May 21, 2024

Module proposal for the Dashboard module: Azure/Azure-Verified-Modules#980

@eriqua
Copy link
Contributor

eriqua commented May 21, 2024

Thanks @zedy-wj for raising this PR and triggering this discussion.

  1. dashboard should be independent module
  2. it could be included in a pattern module in the future

for now, let's get the dashboard module created if not there yet

+1 on the above.
Create a new resource module for dashboard and discuss if adding a pattern module in the future.
A pattern module would make sense if we think the integration can be reusable. If so +1 for the pattern as well. If not, I'd rather keep it covered by e2e deployment examples only.

@matebarabas matebarabas added Status: Do Not Merge ⛔ Do not merge PRs with this label attached as they are not ready or aligned to future direction etc. Class: Resource Module 📦 This is a resource module and removed Needs: Triage 🔍 Maintainers need to triage still labels May 21, 2024
@matebarabas
Copy link
Contributor

Requires approved module proposal and prior publication to the MAR manifest. Do not merge, until both are confirmed here.

@eriqua
Copy link
Contributor

eriqua commented Jun 6, 2024

Sorry if I missed it: what's the plan for this PR?
Is #2016 replacing it?

@zedy-wj
Copy link
Member Author

zedy-wj commented Jun 6, 2024

Sorry if I missed it: what's the plan for this PR? Is #2016 replacing it?

@eriqua - Currently #2016 is dealing with the creation of the dashboard module, this PR did not develop the dashboard as an independent module. We will close this PR and move the plan to fix #1130 to #2016.

@jongio - We will close this PR and move the plan to fix #1130 to #2016.

@zedy-wj zedy-wj closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Class: Resource Module 📦 This is a resource module Status: Do Not Merge ⛔ Do not merge PRs with this label attached as they are not ready or aligned to future direction etc. Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AVM Module Issue]: Application Insight Module should add the creation of 'Microsoft.Portal/dashboards'
7 participants