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: New module azure-stack-hci/cluster module #3364

Open
wants to merge 78 commits into
base: main
Choose a base branch
from
Open

feat: New module azure-stack-hci/cluster module #3364

wants to merge 78 commits into from

Conversation

mbrat2005
Copy link
Contributor

@mbrat2005 mbrat2005 commented Sep 25, 2024 •
edited by AlexanderSehr
Loading

Description

Adds new Azure Stack HCI Cluster AVM module

Includes some helper modules for e2e testing placed here avm\utilities\e2e-template-assets\templates\azure-stack-hci, as I'll also be using them for other HCI resource modules (always needing to deploy an HCI cluster in Azure to test on). If this is not the right location for this sort of shared asset, please let me know...

Pipeline Reference

Pipeline
avm.res.azure-stack-hci.cluster

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

Maintainer checklist:

  • Microsoft.AzureStackHCI RP registered (in order to grant permissions before starting)
  • Service Principal created and granted Azure Resource Bridge Deployment role on the management group (deployment will create subscription-level permissions, but parallel deployments/failures may clean up the required role). Create a secret credential.
  • CI_arbDeploymentAppId
  • CI_arbDeploymentSPObjectId (from Enterprise Application)
  • CI_arbDeploymentServicePrincipalSecret
  • CI_hciResourceProviderObjectId (Get-AzADServicePrincipal -ApplicationId 1412d89f-b8a8-4111-b4fd-e82905cbd85d)

@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 25, 2024
@mbrat2005 mbrat2005 mentioned this pull request Sep 25, 2024
Open
2 tasks
@mbrat2005 mbrat2005 marked this pull request as ready for review September 26, 2024 15:39
@mbrat2005 mbrat2005 requested review from a team as code owners September 26, 2024 15:39
@avm-team-linter avm-team-linter bot added the Needs: Core Team 🧞 This item needs the AVM Core Team to review it label Sep 26, 2024
@eriqua eriqua changed the title feat: azure-stack-hci/cluster module feat: New module azure-stack-hci/cluster module Sep 27, 2024
@ReneHezser
Copy link
Contributor

ReneHezser commented Oct 4, 2024 •
edited
Loading

@mbrat2005
We have added examples for Bicep parameter files to the Readme. This has been applied to all published modules but needs to be done for PRs as well. Can you please update your branch and run the Set-AVMModule utility as detailed here. It is required for the validation pipeline to succeed and the contribution to be published.

Please reach out if any support is needed.

AlexanderSehr
AlexanderSehr reviewed Oct 9, 2024
View reviewed changes
avm/utilities/e2e-template-assets/templates/azure-stack-hci/modules/azureStackHCIHost/README.md Outdated
@@ -0,0 +1,67 @@
# HCI Azure Host Deployment Bicep Module
Copy link
Contributor

Choose a reason for hiding this comment

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

How come you have this readme here? Is this content you wanted to have in the module's README.md?
If so (in case you did not know), you can add a ## Notes section to it (and as many subheaders below it as you want) and add the content there. The readme generation will preserve that section and append it after the generation again to the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more because this code is reusable elsewhere (in labs, etc) and I wanted to document it for easier sharing. I could remove the readme of you prefer...

AlexanderSehr
AlexanderSehr reviewed Nov 1, 2024
View reviewed changes
avm/utilities/pipelines/e2eValidation/resourceDeployment/New-TemplateDeployment.ps1 Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @mbrat2005 what happened here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept getting 'An error occurred while sending the request.' during testing, but have yet to hit it again while this workaround catch is in place--trying again today. I don't want to be getting failed runs every week once there are several modules deploying several tests... I plan to revert this and rework for a separate PR if this works around that issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Understandable. I guess you're referring to the deployment returning that, right? I.e. in the ARM log when it fetches the state of the deployment every 30 seconds or so

AlexanderSehr
AlexanderSehr reviewed Nov 1, 2024
View reviewed changes
avm/res/azure-stack-hci/cluster/tests/e2e/waf-aligned/main.test.bicep
@description('Required. The app ID of the service principal used for the Azure Stack HCI Resource Bridge deployment. If omitted, the deploying user must have permissions to create service principals and role assignments in Entra ID.')
@secure()
#disable-next-line secure-parameter-default
param arbDeploymentAppId string = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a blocking comment for us maintainers until the same secrets are set up

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbrat2005 I may need your help with those secrets. Some sound like I should just have a dedicated service principal set up, while others sound like tenant-specific Enterprise Applications which are quite common, yet I cannot, for example, find one that has a name like Azure Stack HCI Resource Provider. I guess there are a couple like the latter, right?
If the case, it would be great if we could add the exact name to the secret's description to enable folks to set them up by themselves :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexanderSehr The HCI RP identity doesn't show up until you register for the Microsoft.AzureStackHCI RP. Once registered, running this command (or the CLI equivalent) should show the SP ID for the global app in the current tenant: Get-AzADServicePrincipal -ApplicationId 1412d89f-b8a8-4111-b4fd-e82905cbd85d

I'll add the app ID to the param description...

Copy link
Contributor

@AlexanderSehr AlexanderSehr Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @mbrat2005,
I'm really sorry this is taking so long on my end. I'm trying to squeeze working on this PR in every time I get the chance. I'll try to continue on this this week 💪

@jtracey93
Copy link
Contributor

Hey @mbrat2005 ,

Firstly, thanks for your work on this PR!

We have made some changes to the AVM CI, detailed below, which means we need you to update your fork to pull in these latest changes and re-run your tests to show they still are passing prior to approving and merging this PR, as we don't and it fails once merged the publishing of your module will fail and will be blocked going forward until the test pass again via additional PRs.

Changes to CI That Have Been Made That You Need To Take Action On

Any questions reach out to the AVM Core Team by tagging us in your PR here or internally via Teams

Thanks

Jack (AVM Core Team)

AlexanderSehr
AlexanderSehr reviewed Nov 28, 2024
View reviewed changes
avm/res/azure-stack-hci/cluster/tests/e2e/3nodeswitchless.defaults/main.test.bicep
@@ -35,7 +35,7 @@ param arbDeploymentSPObjectId string = ''
#disable-next-line secure-parameter-default
param arbDeploymentServicePrincipalSecret string = ''

@description('Optional. The service principal ID of the Azure Stack HCI Resource Provider. If this is not provided, the module attemps to determine this value by querying the Microsoft Graph.')
@description('Required. The service principal ID of the Azure Stack HCI Resource Provider in this tenant. To find this value, look up the service principal with app ID 1412d89f-b8a8-4111-b4fd-e82905cbd85d after registering the Azure Stack HCI RP in the tenant. For example: `Get-AzADServicePrincipal -ApplicationId 1412d89f-b8a8-4111-b4fd-e82905cbd85d`.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @mbrat2005, what's the name of that application? I can't find an application with that ID (neither in the portal, nor via code)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eriqua eriqua eriqua left review comments

@matebarabas matebarabas matebarabas left review comments

@AlexanderSehr AlexanderSehr AlexanderSehr left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@AlexanderSehr AlexanderSehr

@mbrat2005 mbrat2005

Labels
Class: Resource Module 📦 This is a resource module Needs: Attention 👋 Reply has been added to issue, maintainer to review Needs: Core Team 🧞 This item needs the AVM Core Team to review it Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mbrat2005 @ReneHezser @AlexanderSehr @jtracey93 @matebarabas @eriqua