Skip to content

chore: update resource API versions and improve naming conventions #37

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

Closed
wants to merge 15 commits into from

Conversation

segraef
Copy link

@segraef segraef commented Feb 28, 2025

Purpose

  • ...

Does this introduce a breaking change?

[ ] Yes
[ ] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install
  • Test the code

What to Check

Verify that the following are valid

  • ...

Other Information

segraef and others added 4 commits March 2, 2025 12:51

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Chad Kittel <chad.kittel@gmail.com>
…ng conventions
…esources

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@segraef segraef requested a review from ckittel April 3, 2025 22:40
…hanced functionality
@segraef
Copy link
Author

segraef commented Apr 3, 2025

Copy link
Author

@segraef segraef left a comment

Choose a reason for hiding this comment

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

.

…ecurity and management
Copy link
Member

@ckittel ckittel left a comment

Choose a reason for hiding this comment

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

Thanks for updating API versions and applying some formatting changes.

Before I merge, I'll wait to hear from @segraef -- as he let me know that the latest test had a failure due to something related to a network monitor in Australia east.

@segraef
Copy link
Author

segraef commented Apr 21, 2025

Thanks for updating API versions and applying some formatting changes.

Before I merge, I'll wait to hear from @segraef -- as he let me know that the latest test had a failure due to something related to a network monitor in Australia east.

Hey @ckittel I just made another test-run in westus, eastus and a couple of other regions to identify the property/setting which causes the new network monitor enablement.

Just for understanding, I'm using the same properties you want me to remain with the new api versions what's causing this.

{
  "code": "DeploymentFailed",
  "target": "/subscriptions/subId/resourceGroups/aoai-e2e-basic/providers/Microsoft.Resources/deployments/aiStudio",
  "message": "At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-deployment-operations for usage details.",
  "details": [
    {
      "code": "ValidationError",
      "target": "workspaceDto",
      "message": "Can't enable network monitor in region: eastus."
    }
  ]
}

Testing is very time-intensive since I everytime I do a full e2e deployment which takes about 1h since the aoai workspaces need to be purged before another deployment can run but also to have a full e2e testing done properly since this is an Azure-Sample which should work for anyone else consuming it.

I'll give it a couple of more tries now, otherwise I'm going to close this PR due to my constrained capacity, sorry for that mate.

@segraef
Copy link
Author

segraef commented Apr 21, 2025

Okay, I have a working version using 2024-10-01 but this version does not support (dropped) the previous properties as you can see here which was also the main reason why I removed them:

image

These properties were brought back in 2025-01-01-preview, which this current PR still holds but fails due to

    {
      "code": "ValidationError",
      "target": "workspaceDto",
      "message": "Can't enable network monitor in region: eastus."
    }

For now I'm going to close this PR until a more stable version is being published or you accept my initial PR which brought in a state at 2024-10-01 which has a working version here.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
ckittel and others added 4 commits April 22, 2025 11:47

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@ckittel
Copy link
Member

ckittel commented Apr 24, 2025

Thank you. We've got almost all of these applied now via the other recent PRs (#42, #43, #44) that used your PR as inspiration. The only one we skipped (for now) is App Service. We plan on revisiting that soon, also not going to apply the role assignment one or the couple of formatting tweaks. We might pick the formatting tweaks up in a future PR.

So, we'll close this one as mostly applied. Once again, thank you! <3

@ckittel ckittel closed this Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants