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

fix: Adding certificates child resource and updated max test #4682

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

Conversation

hundredacres
Copy link
Contributor

@hundredacres hundredacres commented Mar 4, 2025

Description

Added certificate child resource, updated the max test to test for it and removed unnecessary deps.

Fixes #3855
Closes #3855

Pipeline Reference

Pipeline
avm.res.app.managed-environment

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

@hundredacres hundredacres requested review from a team as code owners March 4, 2025 21:55
@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 Mar 4, 2025
@avm-team-linter avm-team-linter bot added the Needs: Core Team 🧞 This item needs the AVM Core Team to review it label Mar 4, 2025
params: {
name: certificateObject.?name ?? 'cert-${name}'
managedEnvironmentName: managedEnvironment.name
certificateKeyVaultProperties: certificateKeyVaultProperties
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @hundredacres,
I won't block the PR over it but want to raise one question:
The certificateKeyVaultProperties are technically a property of the certificate child module - and could be part of the certificateObject. However, they're instead currently decleared as a separate parameter.
What I'm wondering is, if it would be better / more intuitive to merge the parameter into the wider certificateObject parameter (that is, merging the two parameters into one), and having line 179 and follwing simply reference the property from this location like

      certificateKeyVaultProperties: !empty(certificateObject.?certificateKeyVaultProperties)
        ? {
            identity: certificateObject!.certificateKeyVaultProperties!.identityResourceId
            keyVaultUrl: certificateObject!.certificateKeyVaultProperties!.keyVaultUrl
          }
        : null

In general it seems a bit confusing that this and other parameters like the certificateValue, certificatePassword, etc. are all duplicated.
Could it be that the child is even the same as the property? If so, it may even be possible to not define the properties in line 175 and following at all, and instead have the child take care of it exclusively.

Just some thoughts. Maybe completely wrong 😄 Just wanted to ask as I got a bit confused over it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @hundredacres,
did you happen to find out anything about it? Do both need to be declared? The PR already looks good as it is as far as I'm concerned. Would just be curious.

@eriqua eriqua added Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author and removed Needs: Triage 🔍 Maintainers need to triage still labels Mar 22, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the Status: No Recent Activity 💤 When an issue/PR has not been modified for X amount of days label Mar 26, 2025
@@ -319,4 +342,4 @@ type storageType = {

@description('Required. File share name.')
shareName: string
}
}?
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
}?
}

Parameter is already (correctly) nullable

@AlexanderSehr
Copy link
Contributor

Hey @hundredacres,
the PR looks pretty much good as is. I added one comment that should be addressed (regarding the nullable), and the other comment you could resolve as it's just a question as I find the resource provider a bit ... unintuitive 😄 But that's obviously not on you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author Needs: Core Team 🧞 This item needs the AVM Core Team to review it Status: No Recent Activity 💤 When an issue/PR has not been modified for X amount of days Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
3 participants