Skip to content

fix: Update avm/res/net-app/net-app-account - fix optional zone for volumes and specify protocolTypes options#4469

Merged
fbinotto merged 17 commits intoAzure:mainfrom
thecmdradama:fix/netapp-pattern
Feb 26, 2025
Merged

fix: Update avm/res/net-app/net-app-account - fix optional zone for volumes and specify protocolTypes options#4469
fbinotto merged 17 commits intoAzure:mainfrom
thecmdradama:fix/netapp-pattern

Conversation

@thecmdradama
Copy link
Contributor

@thecmdradama thecmdradama commented Feb 18, 2025

Description

This PR resolves the following issues in the avm/res/net-app/net-app-account module

  • On the NetApp Volume submodule, zone has been renamed to availabilityZone.
  • The type for availabilityZone has been changed to string from int[] and is now optional with null as the default. Previously the module would try and deploy a volume into all three zones if no value is defined.
  • Changed: protocolTypes is now an optional array of strings with three possible options 'NFSv3', 'NFSv4.1', and 'CIFS'. If no value is provided, the default value of ['NFSv3'] is used.
  • Added to the description for protocolTypes indicating that the user will need to specify ['NFSv3','CIFS'] or ['NFSv4.1','CIFS'] if creating dual-stack volumes.
  • Added in additional submodule README.md and main.json files generated by Set-AVMModule.

Pipeline Reference

Pipeline
avm.res.net-app.net-app-account

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

@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 Feb 18, 2025
@thecmdradama thecmdradama marked this pull request as ready for review February 18, 2025 06:29
@thecmdradama thecmdradama requested review from a team as code owners February 18, 2025 06:29
@avm-organizer avm-organizer bot added the Needs: Module Owner 📣 This module needs an owner to develop or maintain it label Feb 18, 2025
@thecmdradama thecmdradama changed the title fix: Update avm/res/net-app/net-app-account - fix optional zone for volumes and specify protocolTypes options fix: Update avm/res/net-app/net-app-account - fix optional zone for volumes and specify protocolTypes options Feb 18, 2025
@AlexanderSehr
Copy link
Collaborator

AlexanderSehr commented Feb 22, 2025

Hey @fbinotto,
please review at your earliest convenience :)
The PR looks good from my side.

@thecmdradama
Copy link
Contributor Author

thecmdradama commented Feb 26, 2025

Just had a thought around another change I would like to make. Being that the capacity pool size must be set in 1TiB chunks. Is there any value in a user defined function to handle the Tebibyte to byte conversion? Would make consuming the module considerably easier. Thoughts @AlexanderSehr and @fbinotto?

@AlexanderSehr
Copy link
Collaborator

Just had a thought around another change I would like to make. Being that the capacity pool size must be set in 1TiB chunks. Is there any value in a user defined function to handle the Tebibyte to byte conversion? Would make consuming the module considerably easier. Thoughts @AlexanderSehr and @fbinotto?

Hey @thecmdradama,
not to my knowledge and also none is to be found when looking at the available Bicep functions.
However, it is possible to define functions that could do this on the user's behalf.

@thecmdradama
Copy link
Contributor Author

thecmdradama commented Feb 26, 2025

Hey @thecmdradama, not to my knowledge and also none is to be found when looking at the available Bicep functions. However, it is possible to define functions that could do this on the user's behalf.

Thanks @AlexanderSehr, a user defined function was what I was thinking of implementing to then simplify the capacity pool size parameter.
From what I can see, there's only one pattern (subResourceWrapper) and no resources that currently implement UDF's. So not really anything currently to try and create a standard from.

I've thrown a quick example together on how it could look in my last commit. Maybe a topic of discussion for the AVM Core team on an agreed approach for utilising UDF's in modules and patterns?

Thanks,

@AlexanderSehr
Copy link
Collaborator

AlexanderSehr commented Feb 26, 2025

Hey @thecmdradama, not to my knowledge and also none is to be found when looking at the available Bicep functions. However, it is possible to define functions that could do this on the user's behalf.

Thanks @AlexanderSehr, a user defined function was what I was thinking of implementing to then simplify the capacity pool size parameter. From what I can see, there's only one pattern (subResourceWrapper) and no resources that currently implement UDF's. So not really anything currently to try and create a standard from.

I've thrown a quick example together on how it could look, in my last commit. Maybe a topic of discussion for the AVM Core team on an agreed approach for utilising UDF's in modules and patterns?

Thanks,

ah well, no standard means no restrictions 😉
My only thought was that if there were multiple such functions it might make sense to publish them as a utl (utility) module. But for just one function, this would be a bit of an overkill.
Looking at your commit, I'd argue it looks great as it is 💪

@thecmdradama
Copy link
Contributor Author

ah well, no standard means no restrictions 😉

That's when I get worried 😄

My only thought was that if there were multiple such functions it might make sense to publish them as a utl (utility) module. But for just one function, this would be a bit of an overkill.

Yeah, I definitely see value in having a common functions utility module for when others start adopting UDFs, though I don't think many other modules would need this specific one. Only similar one I can see would be elastic-san but that just needs you to pass in tebibytes not tebibyte blocks in bytes...

Looking at your commit, I'd argue it looks great as it is 💪

Thank you, I'll take that compliment :D

Copy link
Contributor

@fbinotto fbinotto left a comment

Choose a reason for hiding this comment

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

LGTM

@fbinotto fbinotto merged commit 13555e6 into Azure:main Feb 26, 2025
4 checks passed
@thecmdradama thecmdradama deleted the fix/netapp-pattern branch February 26, 2025 23:59
fbinotto pushed a commit that referenced this pull request Feb 27, 2025
## Description
Fixes an issue created with the #4469 PR with the referenced parameter
for `zone` on the net-app capacity pool. I had not changed this from
`availabilityZone`.
<!--
>Thank you for your contribution !
> Please include a summary of the change and which issue is fixed.
> Please also include the context.
> List any dependencies that are required for this change.

Fixes #123
Fixes #456
Closes #123
Closes #456
-->

## Pipeline Reference

<!-- Insert your Pipeline Status Badge below -->

| Pipeline |
| -------- |
|          |

## Type of Change

<!-- Use the checkboxes [x] on the options that are relevant. -->

- [ ] Update to CI Environment or utilities (Non-module affecting
changes)
- [x] Azure Verified Module updates:
- [x] 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.
- [ ] My corresponding pipelines / checks run clean and green without
any errors or warnings

<!-- Please keep up to date with the contribution guide at
https://aka.ms/avm/contribute/bicep -->

---------

Co-authored-by: Adam Ricket <adam.ricket@cybercx.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Module Owner 📣 This module needs an owner to develop or maintain it Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants