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

AI Studio terraform templates #322

Merged
merged 10 commits into from
Jun 26, 2024
Merged

Conversation

andyaviles121
Copy link
Contributor

No description provided.

@andyaviles121
Copy link
Contributor Author

@grayzu @stemaMSFT @lonegunmanb can someone take a look at this pr?
cc: @deeikele

Copy link
Contributor

@grayzu grayzu 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 the PR. I have added a few minor comments and questions.

quickstart/101-ai-studio/dependent.tf Outdated Show resolved Hide resolved
quickstart/101-ai-studio/dependent.tf Outdated Show resolved Hide resolved
quickstart/101-ai-studio/dependent.tf Outdated Show resolved Hide resolved
quickstart/101-ai-studio/hub.tf Outdated Show resolved Hide resolved
quickstart/101-ai-studio/main.tf Outdated Show resolved Hide resolved
quickstart/101-ai-studio/project.tf Outdated Show resolved Hide resolved
quickstart/101-ai-studio/variables.tf Outdated Show resolved Hide resolved
@andyaviles121
Copy link
Contributor Author

@grayzu changes have been made.

@andyaviles121 andyaviles121 requested a review from grayzu May 17, 2024 20:21
@grayzu
Copy link
Contributor

grayzu commented May 17, 2024

Looks good to me.

@andyaviles121
Copy link
Contributor Author

@grayzu when can this be merged?

@stemaMSFT
Copy link
Member

@andyaviles121 please fix the issues with the PR. When it passes e2e tests we can merge.

@stemaMSFT
Copy link
Member

stemaMSFT commented May 21, 2024

@lonegunmanb looks like this e2e is failing because of some sort of race condition or deadlock based on the logs. Can you take a look?

@andyaviles121
Copy link
Contributor Author

@lonegunmanb have you had a chance to look over this pr? I'm not sure how to fix issues with the e2e check failure

@andyaviles121
Copy link
Contributor Author

@lonegunmanb is the issue due to the reused resource names? Seems like the key vault is having issues being created.
cc: @deeikele

@andyaviles121
Copy link
Contributor Author

@stemaMSFT can you try to run the e2e test again? I added in a section to the azurerm provider to hopefully help with the key vault issues.

@deeikele
Copy link

@stemaMSFT Can you please help unblock this E2E build? This seems not triggerable on our side. We have several customers looking for these templates to be merged? Thanks!

@stemaMSFT
Copy link
Member

Sorry for letting this slip for a few weeks. Just kicked off those E2E tests.

@stemaMSFT
Copy link
Member

@andyaviles121 @deeikele @lonegunmanb we're still running into E2E test issues.

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Hi @andyaviles121 @deeikele thanks for opening this pr and apology for this late response.

I've checked the error message:

Test_Quickstarts/quickstart/101-ai-studio 2024-06-14T00:35:53Z command.go:185: │ Key Vault Name: "tftemplatekeyvault"): performing CreateOrUpdate: vaults.VaultsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="VaultAlreadyExists" Message="The vault name 'tftemplatekeyvault' is already in use. Vault names are globally unique so it is possible that the name is already taken. If you are sure that the vault name was not taken then it is possible that a vault with the same name was recently deleted but not purged after being placed in a recoverable state. If the vault is in a recoverable state then the vault will need to be purged before reusing the name. For more information about VaultAlreadyExists, soft delete and purging a vault follow this link https://go.microsoft.com/fwlink/?linkid=2147740."

I've left a review comment to solve this issue. @stemaMSFT

quickstart/101-ai-studio/dependent.tf Outdated Show resolved Hide resolved
@stemaMSFT
Copy link
Member

@andyaviles121 @lonegunmanb the e2e tests have failed again, could you take a look?

@andyaviles121
Copy link
Contributor Author

@stemaMSFT seemed like the connection had an incorrect field. I removed it. Can you try the e2e test again?

@TomArcherMsft
Copy link
Collaborator

cc: @stemaMSFT

@andyaviles121 I'm running the e2e tests now.

@TomArcherMsft
Copy link
Collaborator

cc: @grayzu, @stemaMSFT. @lonegunmanb

@andyaviles121 This sample is failing in part because it doesn't adhere to the Contributor Guide requirements for Terraform samples. https://review.docs.microsoft.com/en-us/help/contribute/global-terraform-template?branch=main

@andyaviles121
Copy link
Contributor Author

@TomArcherMsft I rewrote the files to match the contributor guide. Can you review and run the e2e tests?

@TomArcherMsft
Copy link
Collaborator

@andyaviles121 This looks good. Please change "output.tf" to "outputs.tf" and I'll run the e2e tests. Thanks.

@andyaviles121
Copy link
Contributor Author

@TomArcherMsft done :)

@TomArcherMsft
Copy link
Collaborator

TomArcherMsft commented Jun 21, 2024

CC: @andyaviles121

@stemaMSFT @lonegunmanb I ran the e2e tests and it looks as though everything passes now:
https://github.com/Azure/terraform/actions/runs/9616809065/job/26527170852?pr=322

Can this PR be merged now?

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

LGTM!

@lonegunmanb lonegunmanb merged commit c015ce0 into Azure:master Jun 26, 2024
3 checks passed
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.

6 participants