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.

Sign up for GitHub

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

Jump to bottom

feat: Add new ptn modules azd/monitoring and azd/insights-dashboard. #3150

Closed
wants to merge 10 commits into from
Closed

feat: Add new ptn modules azd/monitoring and azd/insights-dashboard. #3150

wants to merge 10 commits into from

Conversation

zedy-wj
Copy link
Member

@zedy-wj zedy-wj commented Sep 2, 2024 •
edited
Loading

Description

Fixes Azure/Azure-Verified-Modules#1206 and Azure/Azure-Verified-Modules#1226.

Pipeline Reference

Pipeline
avm.ptn.azd.insights-dashboard
avm.ptn.azd.monitoring

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

@jongio for notification.

@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 Sep 2, 2024
@zedy-wj
Copy link
Member Author

zedy-wj commented Sep 2, 2024

This PR contains the development of two ptn modules: azd/monitoring and azd/insights-dashboard, and since these two modules have dependencies, I had to put them in one PR for review.

Currently, module ptn/azd/monitoring will reference ptn/azd/insights-dashboard, and if a user reference ptn/azd/insights-dashboard individually, they also need to pass it the relevant parameters of the logAnalytics existing resource. We are not sure whether such a scenario exists at present. In short, the only difference between the two modules is:

  • azd/monitoring: Create logAnalytics resource in the module and pass its id to azd/insights-dashboard.
  • azd/insights-dashboard: Pass the id of an existing logAnalytics resource directly.

So, should we consider merging the two into one module? Do you have any ideas? @jongio

@zedy-wj
Copy link
Member Author

zedy-wj commented Sep 2, 2024 •
edited
Loading

There are currently three errors in static validation in Pipeline:

Error: [-] [azd/insights-dashboard] Owning team should be specified correctly in CODEWONERS file. 11ms (10ms|2ms)
Error: [-] [azd/insights-dashboard] Telemetry deployment should have expected telemetry identifier. 114ms (113ms|1ms)
Error: [-] [azd/insights-dashboard] Module identifier should be listed in issue template in the correct alphabetical position. 32ms (31ms|1ms)

@jongio - Could you help me with the first error?

Error: [-] [azd/insights-dashboard] Owning team should be specified correctly in CODEWONERS file. 11ms (10ms|2ms)

I'm not sure what to fill in the Owning team here, you can refer to this file: https://github.com/Azure/bicep-registry-modules/blob/main/.github/CODEOWNERS

@AlexanderSehr - Could you help take a look at the remaining two errors? What do I need to do to fix them. Thank you very much!

Error: [-] [azd/insights-dashboard] Telemetry deployment should have expected telemetry identifier. 114ms (113ms|1ms)
Error: [-] [azd/insights-dashboard] Module identifier should be listed in issue template in the correct alphabetical position. 32ms (31ms|1ms)

@AlexanderSehr
Copy link
Contributor

liste

Hey @zedy-wj,
regarding the telemetry etc. - can you confirm you created an AVM issue for both modules and that they got approved? If the case, @matebarabas should've updated this file, which is ultimately the one that contains the telemetry value the test expects.

Regarding the order, please make sure the module is listed in the module issues file here as defined in the specs.

@zedy-wj
Copy link
Member Author

zedy-wj commented Sep 3, 2024

liste

Hey @zedy-wj, regarding the telemetry etc. - can you confirm you created an AVM issue for both modules and that they got approved? If the case, @matebarabas should've updated this file, which is ultimately the one that contains the telemetry value the test expects.

Regarding the order, please make sure the module is listed in the module issues file here as defined in the specs.

It worked for me, thanks a lot! 😃

@v-xuto
Copy link
Member

v-xuto commented Sep 3, 2024

@jongio - Could you help me with the first error?

Error: [-] [azd/insights-dashboard] Owning team should be specified correctly in CODEWONERS file. 11ms (10ms|2ms)

@jongio Any ideas about this error?

@AlexanderSehr
Copy link
Contributor

@jongio - Could you help me with the first error?

Error: [-] [azd/insights-dashboard] Owning team should be specified correctly in CODEWONERS file. 11ms (10ms|2ms)

@jongio Any ideas about this error?

This is just about this file. An entry for each respective application must be added there, and the teams created. For context: Based on this file, GitHub will request the module owner's team to review PRs for their modules. And by being part of that team (and it being created in the correct way), owners can approve set PRs.

If this news to you, I'd strongly encourage you to read through the AVM specs starting from here (all the way through until you hit the CODEOWNERS file header.

@zedy-wj zedy-wj marked this pull request as ready for review September 3, 2024 09:09
@zedy-wj zedy-wj requested review from a team as code owners September 3, 2024 09:09
@zedy-wj
Copy link
Member Author

zedy-wj commented Sep 3, 2024

@jongio - Corresponding pipelines / checks run clean and green. Please review this PR, thanks.

@matebarabas
Copy link
Contributor

Hey @zedy-wj, regarding the telemetry etc. - can you confirm you created an AVM issue for both modules and that they got approved? If the case, @matebarabas should've updated this file, which is ultimately the one that contains the telemetry value the test expects.

@AlexanderSehr, I can confirm these modules are on the approved list:

FYI, this is the correct CSV file that holds them (as these are pattern modules).

AlexanderSehr
AlexanderSehr reviewed Sep 4, 2024
View reviewed changes
avm/ptn/azd/monitoring/main.bicep
}

// module applicationInsights 'br/public:avm/ptn/azd/insights-dashboard:version'
module applicationInsights '../insights-dashboard/main.bicep' = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this implemented in a separate module? If you want to reference the dashboard module - then we have to apply a different approach. As per the specs, no local file references (that is, outside the module folder) are allowed (as this leads to a variety of issues such as cascading breaking changes). To this end, please move the monitoring module out of this PR and into a seperate one. We'd then merge insights-dashboard first and publish it, and then you could enable line 63 (the reference to the published module).

AlexanderSehr
AlexanderSehr reviewed Sep 4, 2024
View reviewed changes
avm/ptn/azd/monitoring/main.bicep
}
}

module logAnalytics 'modules/loganalytics.bicep' = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a seperate module if the scope is 1:1 the same as the parent? 😄 As a general recommendation I'd suggest to minimize the number of deployments, unless there's a good reason to have them

AlexanderSehr
AlexanderSehr reviewed Sep 4, 2024
View reviewed changes
avm/ptn/azd/monitoring/main.bicep
output applicationInsightsConnectionString string = applicationInsights.outputs.connectionString

@description('The resource ID of the application insights.')
output applicationInsightsId string = applicationInsights.outputs.id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
output applicationInsightsId string = applicationInsights.outputs.id
output applicationInsightsResourceId string = applicationInsights.outputs.id

AlexanderSehr
AlexanderSehr reviewed Sep 4, 2024
View reviewed changes
avm/ptn/azd/monitoring/main.bicep
output applicationInsightsName string = applicationInsights.outputs.name

@description('The resource ID of the loganalytics workspace.')
output logAnalyticsWorkspaceId string = logAnalytics.outputs.id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
output logAnalyticsWorkspaceId string = logAnalytics.outputs.id
output logAnalyticsWorkspaceResourceId string = logAnalytics.outputs.id

AlexanderSehr
AlexanderSehr reviewed Sep 4, 2024
View reviewed changes
avm/ptn/azd/monitoring/main.bicep
metadata owner = 'Azure/module-maintainers'

@description('Required. The resource portal dashboards name.')
param applicationInsightsDashboardName string = ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If required, it should not have a default

@zedy-wj
Copy link
Member Author

zedy-wj commented Sep 5, 2024

@AlexanderSehr - We created a new PR for azd/insights-dashboard: #3175. After it is merged, we will create a new PR for azd/monitoring. We're closing this PR for now. Thanks for your review!

@zedy-wj zedy-wj closed this Sep 5, 2024
@matebarabas matebarabas added the AZD 🧑‍💻 These modules are requested/used by the AZD team. label Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexanderSehr AlexanderSehr AlexanderSehr left review comments

Assignees
No one assigned
Labels
AZD 🧑‍💻 These modules are requested/used by the AZD team. Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Module Proposal]: avm/ptn/azd/insights-dashboard
4 participants
@zedy-wj @AlexanderSehr @v-xuto @matebarabas