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

New Resource: azurerm_cognitive_account_rai_policy #28013

Merged
merged 10 commits into from
Jan 30, 2025

Conversation

liuwuliuyun
Copy link
Contributor

@liuwuliuyun liuwuliuyun commented Nov 14, 2024

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.

Adding a new resource in Cognitive RP - azurerm_cognitive_account_rai_policy.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Screenshot 2024-11-14 142034
image
Screenshot 2024-11-14 141626

Note that the custom block list hasn't been integrated into the AzureRM provider yet, but it's possible to implement it using the AzAPI provider for the time being. I plan to incorporate it into the complete test case as soon as the azurerm_cognitive_account_custom_blocklist resource is added in the provider.

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #22822

katbyte
katbyte previously approved these changes Dec 4, 2024
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🚜

internal/services/cognitive/client/client.go Outdated Show resolved Hide resolved
sku_name = "S0"
}

resource "azurerm_cognitive_account_rai_policy" "test" {
Copy link
Member

Choose a reason for hiding this comment

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

custom_blocklist is missing here?

Copy link
Contributor Author

@liuwuliuyun liuwuliuyun Dec 10, 2024

Choose a reason for hiding this comment

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

The custom blocklist resource is currently not supported in AzureRM. Here is a PR to add support for this resource. I will also update the test case to include the custom blocklist resource in a seperate PR once that PR got merged. This property is included here because it can be created and used with the AzAPI provider. Per that PR's discussion, should we change this property to the id of custom blocklist?

Copy link
Member

Choose a reason for hiding this comment

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

The order in which these resources have been added is backwards then since properties that accept other resources as input should be referred to by their IDs instead of a name string.

Copy link
Member

Choose a reason for hiding this comment

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

There's no update test here?

Copy link
Contributor Author

@liuwuliuyun liuwuliuyun Dec 10, 2024

Choose a reason for hiding this comment

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

Because all the changable property are already tested in the complete test case. Also followed guidance. I will add a update test case here using complete template to update. In this case, the complete test case is no longer needed right?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for linking the guidance @liuwuliuyun. The acceptance test section of that guide is updated, I submitted a PR to update it #28241

@@ -56,5 +56,6 @@ func (r Registration) Resources() []sdk.Resource {
return []sdk.Resource{
AzureAIServicesResource{},
CognitiveDeploymentResource{},
CognitiveAccountRaiPolicyResource{},
Copy link
Member

Choose a reason for hiding this comment

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

Alphabetical ordering, please move this to line 58

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do


* `id` - The ID of the Cognitive Service Account RAI Policy.

* `type` - The type of the Cognitive Service Account RAI Policy. Possible values are `SystemManaged` or `UserManaged`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `type` - The type of the Cognitive Service Account RAI Policy. Possible values are `SystemManaged` or `UserManaged`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove this

Comment on lines 101 to 104
* `create` - (Defaults to 30 minutes) Used when creating the Cognitive Service Account.
* `update` - (Defaults to 30 minutes) Used when updating the Cognitive Service Account.
* `read` - (Defaults to 5 minutes) Used when retrieving the Cognitive Service Account.
* `delete` - (Defaults to 30 minutes) Used when deleting the Cognitive Service Account.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `create` - (Defaults to 30 minutes) Used when creating the Cognitive Service Account.
* `update` - (Defaults to 30 minutes) Used when updating the Cognitive Service Account.
* `read` - (Defaults to 5 minutes) Used when retrieving the Cognitive Service Account.
* `delete` - (Defaults to 30 minutes) Used when deleting the Cognitive Service Account.
* `create` - (Defaults to 30 minutes) Used when creating the Cognitive Service Account RAI Policy.
* `update` - (Defaults to 30 minutes) Used when updating the Cognitive Service Account RAI Policy.
* `read` - (Defaults to 5 minutes) Used when retrieving the Cognitive Service Account RAI Policy.
* `delete` - (Defaults to 30 minutes) Used when deleting the Cognitive Service Account RAI Policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change

@liuwuliuyun
Copy link
Contributor Author

Rerun all tests:
image
image
image

@stephybun
Copy link
Member

Dependent on #28043 going in first so custom_blocklist can be tested.

@stephybun
Copy link
Member

@liuwuliuyun #28043 has gone in, can you please rebase and update your test case to include custom_blocklist?

# Conflicts:
#	internal/services/cognitive/client/client.go
#	vendor/modules.txt
@liuwuliuyun
Copy link
Contributor Author

I have identified a design issue when including the custom_blocklist.

Whether we use an ID or name, such as when the user sets rai_blocklist_id = azurerm_cognitive_account_rai_blocklist.test.id in azurerm_cognitive_account_rai_policy.test, it creates a dependency from azurerm_cognitive_account_rai_policy.test to azurerm_cognitive_account_rai_blocklist.test.

This dependency causes Terraform to wait for azurerm_cognitive_account_rai_blocklist.test to finish deleting before deleting azurerm_cognitive_account_rai_policy.test. However, this sequence is not allowed by the API and results in an error: Please remove references for rai_blocklist then delete it.

Additionally, manually adding depends_on = [azurerm_cognitive_account_rai_policy.test] in the resource azurerm_cognitive_account_rai_blocklist.test creates a circular dependency.

@stephybun, do you have any suggestions on how to resolve this?

@liuwuliuyun
Copy link
Contributor Author

To address the issue I mentioned, I have removed the custom_blocklist for this resource and intend to create a new resource azurerm_cognitive_account_rai_policy_custom_blocklist_association to handle this special case. @stephybun What do you think?

@catriona-m
Copy link
Member

To address the issue I mentioned, I have removed the custom_blocklist for this resource and intend to create a new resource azurerm_cognitive_account_rai_policy_custom_blocklist_association to handle this special case. @stephybun What do you think?

Hi @liuwuliuyun - after discussion with the rest of the team, we think your suggested approach should work here. Are there any more changes required on this PR for this to work or is this ready to be re-reviewed?

@liuwuliuyun
Copy link
Contributor Author

To address the issue I mentioned, I have removed the custom_blocklist for this resource and intend to create a new resource azurerm_cognitive_account_rai_policy_custom_blocklist_association to handle this special case. @stephybun What do you think?

Hi @liuwuliuyun - after discussion with the rest of the team, we think your suggested approach should work here. Are there any more changes required on this PR for this to work or is this ready to be re-reviewed?

No more changes if you could agree on this approach, thanks.

Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Thanks @liuwuliuyun LGTM!

@catriona-m catriona-m dismissed stephybun’s stale review January 30, 2025 12:01

comments addressed

@catriona-m catriona-m merged commit 9375225 into hashicorp:main Jan 30, 2025
33 checks passed
@github-actions github-actions bot added this to the v4.17.0 milestone Jan 30, 2025
catriona-m added a commit that referenced this pull request Jan 30, 2025
jackofallops added a commit that referenced this pull request Jan 31, 2025
* Update for #27950

* Update for #27824

* Update for #28592 #28583 #28599 #28590 #28453

* Update #28528

* Update for #27853

* Update CHANGELOG.md #28221

* Update for #27760

* Update CHANGELOG.md #28480

* Update CHANGELOG.md typo

* Update CHANGELOG.md #28372

* Update CHANGELOG.md for #26047

also alphabetised ENHANCEMENTS

* Update for #28146

* Update CHANGELOG.md #28013

* Update CHANGELOG.md for #28492

* Update CHANGELOG.md for #28648

* Update CHANGELOG.md for #28549

* Update for #28469 #28620

* prep for release

* i touched it last

---------

Co-authored-by: catriona-m <[email protected]>
Co-authored-by: jackofallops <[email protected]>
Co-authored-by: sreallymatt <[email protected]>
Co-authored-by: Matthew Frahry <[email protected]>
Co-authored-by: jackofallops <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for content filters in Azure OpenAI
5 participants