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

feat: additional parameters for anf #2695

Merged
merged 26 commits into from
Jul 16, 2024

Conversation

bobmclane999
Copy link
Contributor

Description

Additional parameters for NetApp -

  • Active Directory encryption, Kerberos, ldapoverTLS
  • Volumes - Cool Access, Zones, replication,
  • Backup Policies, Vaults & backups

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

@bobmclane999 bobmclane999 requested review from a team as code owners July 10, 2024 16:22
@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 Jul 10, 2024
@bobmclane999 bobmclane999 changed the title Feat additional parameters for anf feat: additional parameters for anf Jul 10, 2024
@bobmclane999
Copy link
Contributor Author

bobmclane999 commented Jul 10, 2024

@jtracey93 Please do you mind give this PR the once over ? As only 2nd one :) I presume I can't resolve the conflicts but most are changes to the json/readme files post Set-AVVMModule..?
@fbinotto Please can you review as soon as you can so we can crack on with our File Share Migration ANF deployments :)

Thanks guys!

@jtracey93
Copy link
Contributor

hey @bobmclane999, i think you ned to pull in and merge and resolve those merge conflicts. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line

Something like:

git fetch upstream
git merge upstream main
<RESOLVE CONFLICTS>
git commit
git push

<RE RUN SET-AVMMODULE SCRIPT>
git commit
git push

@fbinotto
Copy link
Contributor

Take your time @bobmclane999 and @jtracey93

I still have to merge #2636 before I consider this PR. And that PR has some of the changes proposed here.

@bobmclane999
Copy link
Contributor Author

bobmclane999 commented Jul 11, 2024

Hi @fbinotto yeah we had the issues with the resourceId duplication on the WAF aligned tests similar to #2636 so had to make a similar change to get the test working -
image

Sorry to push :( but do you have a timescale on approving the other PR (well and this one :)) as we are mid project for a File Migration using ANF so deadlines are a little tight hence all the changes... tyvm Shaun

@bobmclane999 bobmclane999 force-pushed the feat--additional-parameters-for-ANF branch from 57b0f6c to ce325e3 Compare July 11, 2024 10:21
@krbar krbar added the Needs: Module Owner 📣 This module needs an owner to develop or maintain it label Jul 11, 2024
@fbinotto
Copy link
Contributor

Hi @fbinotto yeah we had the issues with the resourceId duplication on the WAF aligned tests similar to #2636 so had to make a similar change to get the test working - image

Sorry to push :( but do you have a timescale on approving the other PR (well and this one :)) as we are mid project for a File Migration using ANF so deadlines are a little tight hence all the changes... tyvm Shaun

It should be merged today or tomorrow, then you should be able to update your PR and I can approve.

@jtracey93
Copy link
Contributor

Hey @bobmclane999, I see @fbinotto's PR is now merged thanks to @AlexanderSehr's approval 👍

You can now pull in these changes into your branch and then re-run the test and README generation etc. and then @fbinotto should be good to review and get this one approved and merged too 👍

@fbinotto
Copy link
Contributor

@bobmclane999 please do so before tomorrow night as I will be on leave for a week.

@jtracey93
Copy link
Contributor

@fbinotto are you happy for me to approve in your absence if needed?

@bobmclane999
Copy link
Contributor Author

@fbinotto are you happy for me to approve in your absence if needed?

Thanks @jtracey93 . @fbinotto - Yeah that would be really appreciated as although I've done this it may need tweaking and I'm supposed to be OOTO today :)

Thanks both for all your help with this!! Sorry still a bit of a newb at this.

@jtracey93
Copy link
Contributor

@fbinotto looks like its good for a review, lets see if we can get it in before you go, if not let me know

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. Just one minor change and we can merge,

avm/res/net-app/net-app-account/capacity-pool/main.bicep Outdated Show resolved Hide resolved
@fbinotto
Copy link
Contributor

@fbinotto are you happy for me to approve in your absence if needed?

Yes, go ahead. I have requested only a minor change.

@bobmclane999
Copy link
Contributor Author

@fbinotto / @jtracey93 - have put networkFeatures and Zones params back and previous and submitted the PR again for review... thanks muchly Shaun :)
Have a great week off @fbinotto !

@jtracey93
Copy link
Contributor

@bobmclane999 did you re-run the Set-AVMModule script post making those changes and re run the tests?

@bobmclane999
Copy link
Contributor Author

@bobmclane999 did you re-run the Set-AVMModule script post making those changes and re run the tests?

Hi @jtracey93 actually no I didn't let me do and let you know when I've re-submitted tyvm

@bobmclane999
Copy link
Contributor Author

Hi @jtracey93 - re-ran a test after another commit (post Set-AVMModule) and have another badge here -

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

Is this the correct place to put it ?

@jtracey93 jtracey93 merged commit 3740074 into Azure:main Jul 16, 2024
4 checks passed
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.

5 participants