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: module avm/res/network/virtual-network-gateway #3639

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

feat: module avm/res/network/virtual-network-gateway #3639

wants to merge 23 commits into from
+135 −39

Conversation

fabmas
Copy link
Member

@fabmas fabmas commented Oct 25, 2024 •
edited
Loading

Description

Added input requested in #3493

Closes #3493

Pipeline Reference

avm.res.network.virtual-network-gateway

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

@fabmas fabmas requested review from a team as code owners October 25, 2024 18:20
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Triage 🔍 Maintainers need to triage still label Oct 25, 2024
@avm-team-linter avm-team-linter bot added the Needs: Core Team 🧞 This item needs the AVM Core Team to review it label Oct 25, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue label Oct 25, 2024
@fabmas fabmas requested a review from eriqua October 25, 2024 18:27
@fabmas fabmas enabled auto-merge (squash) October 25, 2024 19:30
eriqua
eriqua reviewed Oct 30, 2024
View reviewed changes
avm/res/network/virtual-network-gateway/main.bicep Outdated
@@ -494,6 +494,28 @@ output activeActive bool = virtualNetworkGateway.properties.activeActive
@description('The location the resource was deployed into.')
output location string = virtualNetworkGateway.location

@description('The ASN (Autonomous System Number) of the virtual network gateway.')
output asn int? = isBgp ? virtualNetworkGateway.properties.bgpSettings.asn : null //'Not applicable (No Bgp)'
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be enough

Suggested change
output asn int? = isBgp ? virtualNetworkGateway.properties.bgpSettings.asn : null //'Not applicable (No Bgp)'
output asn int? = virtualNetworkGateway.properties.bgpSettings.asn

Copy link
Contributor

Choose a reason for hiding this comment

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

In general instead of providing multiple top level properties I wonder if cleaner to provide a bgp object as output

eriqua
eriqua reviewed Oct 30, 2024
View reviewed changes
avm/res/network/virtual-network-gateway/main.bicep
output asn int? = isBgp ? virtualNetworkGateway.properties.bgpSettings.asn : null //'Not applicable (No Bgp)'

@description('The public IP address of the virtual network gateway.')
output publicIpAddress string = !empty(existingFirstPipResourceId) ? reference(existingFirstPipResourceId,'2023-04-01').ipAddress: publicIPAddress[0].outputs.ipAddress
Copy link
Contributor

Choose a reason for hiding this comment

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

can't this be retrieved as a virtualNetworkGateway property? No matter if existing or not before the deployment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also in this case we could provide primary and secondary pip as part of a publicIp output object

eriqua
eriqua reviewed Oct 31, 2024
View reviewed changes
avm/res/network/virtual-network-gateway/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.

Just a heads up as I noticed your test pipeline failed.
A new test is checking weather Required parameters have a default value. In that case it fails, since if a parameter has a default value is Optional by definition.
This is the case for the skuName parameter.
Changing the description to Optional should fix the issue

@eriqua eriqua self-assigned this Nov 2, 2024
@eriqua eriqua removed the Needs: Triage 🔍 Maintainers need to triage still label Nov 2, 2024
@eriqua eriqua added Type: Bug 🐛 Something isn't working Class: Resource Module 📦 This is a resource module labels Nov 2, 2024
AlexanderSehr
AlexanderSehr reviewed Nov 8, 2024
View reviewed changes
avm/res/network/virtual-network-gateway/README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note, the main.json templates must be re-build with the latest Bicep version 0.31.34 as this version (running on the agents) renders a different main.json file then previous versions.

The steps to execute would be

  • Upgrade your Bicep CLI version to 0.31.34 (e.g. via choco upgrade bicep if choco is used)
  • Re-run the Set-AVMModule script on your module. E.g. via Set-AVMModule -ModuleFolderPath '.\avm\res\automation\automation-account\' -SkipFileAndFolderSetup -SkipReadMe -Recurse
  • Push the changes
  • Re-run the tests to ensure everything works.

If the static tests continue to fail for you, please note that there is currently presumably another new bug which affects some of our modules (ref).

@jtracey93
Copy link
Contributor

Hey @fabmas ,

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)

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

@AlexanderSehr AlexanderSehr AlexanderSehr left review comments

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

Assignees

@fabmas fabmas

@eriqua eriqua

Labels
Class: Resource Module 📦 This is a resource module Needs: Core Team 🧞 This item needs the AVM Core Team to review it Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: Bug 🐛 Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AVM Module Issue]: Output ASN, private + public IPs res/network/virtual-network-gateway
4 participants
@fabmas @jtracey93 @AlexanderSehr @eriqua