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

Api and sdk updates for bicep client #44181

Merged
merged 20 commits into from
Jun 11, 2024
Merged

Api and sdk updates for bicep client #44181

merged 20 commits into from
Jun 11, 2024

Conversation

anamikapan
Copy link
Member

@anamikapan anamikapan commented May 22, 2024

Requested changes are related to swagger - Azure/azure-rest-api-specs#28204

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Mgmt This issue is related to a management-plane library. labels May 22, 2024
Copy link

Thank you for your contribution @anamikapan! We will review the pull request and get back to you soon.

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.ResourceManager.Resources

Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

@anamikapan :

Your account lacks the required public GitHub organizations and permissions required of an internal contributor. Please review the Azure SDK onboarding documentation and use the associated Teams channel for support.

You can verify the state of your account by running the Validate-AzsdkCodeOwner script from the Azure SDK tools repository.

Please also be sure to add yourself to CODEOWNERS for this library, if you will be maintaining it going forward.

@anamikapan
Copy link
Member Author

@jsquire I have requested the specific permissions. Also, the build failures on the PR are because of some spell checks and words that are a part of swagger changes. I have no control over that. Can we manually try to override the spell check errors ?

@jsquire
Copy link
Member

jsquire commented May 23, 2024

@anamikapan : You now have the correct permissions; however you still have not made your Azure and Microsoft org memberships public. Please use the script that I referenced above to validate your account after updating your settings.

@ArthurMa1978 : Can you confirm whether or not your team will own this library going forward?

@jsquire
Copy link
Member

jsquire commented May 23, 2024

Can we manually try to override the spell check errors ?

You'll want to update the cspell config with exceptions specific to this library. Please follow the existing pattern in the file.

@anamikapan
Copy link
Member Author

@jsquire should cspell config additions be part of the same PR? or should that go in before this. can you please clarify

@ArthurMa1978
Copy link
Member

@anamikapan : You now have the correct permissions; however you still have not made your Azure and Microsoft org memberships public. Please use the script that I referenced above to validate your account after updating your settings.

@ArthurMa1978 : Can you confirm whether or not your team will own this library going forward?

No, the service team is responsible for managing the library, and we will assist with the release process.

@jsquire
Copy link
Member

jsquire commented May 24, 2024

@anamikapan : You now have the correct permissions; however you still have not made your Azure and Microsoft org memberships public. Please use the script that I referenced above to validate your account after updating your settings.
@ArthurMa1978 : Can you confirm whether or not your team will own this library going forward?

No, the service team is responsible for managing the library, and we will assist with the release process.

Thanks, Arthur.

@anamikapan: Arthur's response indicates that you will also be required to provide CODEOWNERS data for this PR to move forward.

@jsquire
Copy link
Member

jsquire commented May 24, 2024

@jsquire should cspell config additions be part of the same PR? or should that go in before this. can you please clarify

Please include them as part of this PR.

@jsquire
Copy link
Member

jsquire commented May 28, 2024

Account issues have been resolved. CODEOWNERS content is still required to move forward.

@majastrz
Copy link
Member

@jsquire I'm one of the engineering managers for Azure Deployments. The ARM package (sdk/resources/Azure.ResourceManager.Resources/**) that is modified by @anamikapan in this PR is owned by multiple teams within the ARM organization. I'm perfectly happy to take ownership for the parts of the package that my team owns and add ourselves to the list but we cannot be responsible for the whole package.

The parts that we can own are the paths related to the following resource types (and any nested resource types below them) in the Microsoft.Resources namespace:

  • deployments
  • templateSpecs
  • builtInTemplateSpecs
  • deploymentScripts
  • deploymentStacks
  • decompileBicep

@jsquire
Copy link
Member

jsquire commented May 29, 2024

@jsquire I'm one of the engineering managers for Azure Deployments. The ARM package (sdk/resources/Azure.ResourceManager.Resources/**) that is modified by @anamikapan in this PR is owned by multiple teams within the ARM organization. I'm perfectly happy to take ownership for the parts of the package that my team owns and add ourselves to the list but we cannot be responsible for the whole package.

The parts that we can own are the paths related to the following resource types (and any nested resource types below them) in the Microsoft.Resources namespace:

  • deployments
  • templateSpecs
  • builtInTemplateSpecs
  • deploymentScripts
  • deploymentStacks
  • decompileBicep

Thanks, @majastrz. There will need to be a CODEOWNERS entry added as part of this PR around L965 in the management package section which follows the existing format. Since a library is represented by a single label for issue support purposes, I'd suggest that you create a GitHub team and use that as the owner so that you can more easily manage adding/removing contacts from it.

@majastrz
Copy link
Member

Thanks, @majastrz. There will need to be a CODEOWNERS entry added as part of this PR around L965 in the management package section which follows the existing format. Since a library is represented by a single label for issue support purposes, I'd suggest that you create a GitHub team and use that as the owner so that you can more easily manage adding/removing contacts from it.

@jsquire We have an existing team (@Azure/bicep-admins) that covers Azure Deployments functionality. Can we specify multiple teams?

@jsquire
Copy link
Member

jsquire commented May 29, 2024

Thanks, @majastrz. There will need to be a CODEOWNERS entry added as part of this PR around L965 in the management package section which follows the existing format. Since a library is represented by a single label for issue support purposes, I'd suggest that you create a GitHub team and use that as the owner so that you can more easily manage adding/removing contacts from it.

@jsquire We have an existing team (@Azure/bicep-admins) that covers Azure Deployments functionality. Can we specify multiple teams?

I believe so. @JimSuplizio - can you confirm?

.github/CODEOWNERS Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
@JimSuplizio
Copy link
Member

Since a library is represented by a single label for issue support purposes, I'd suggest that you create a GitHub team and use that as the owner so that you can more easily manage adding/removing contacts from it.

So long as the team is under the azure-sdk-write team it'll get expanded into the individual users.

.github/CODEOWNERS Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
@jsquire jsquire dismissed their stale review June 10, 2024 13:47

CODEOWNERS content confirmed good.

@ArthurMa1978 ArthurMa1978 merged commit 684b650 into Azure:main Jun 11, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants