New Resource: `azurerm_video_indexer_account` by mbfrahry · Pull Request #27632 · hashicorp/terraform-provider-azurerm

mbfrahry · 2024-10-11T21:45:54Z

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

This PR adds azurerm_video_indexer_account as a new resource.

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.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.

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)

Change Log

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

New Resource: azurerm_video_indexer_account [GH-00000]

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 #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

stephybun

Thanks @mbfrahry! Could you take a look at the comments I left in-line?

stephybun · 2024-10-15T13:50:59Z

internal/services/appservice/linux_function_app_resource_test.go

    secret_permissions = [
      "Get",
-      "List",
+      "List", l


Accident? :o

stephybun · 2024-10-15T13:53:13Z

internal/services/videoindexer/account_resource.go

+}
+
+type StorageModel struct {
+	StorageAccountId       string `tfschema:"storage_account_id"`


Should this just be named account_id since it's already under a storage block?

stephybun · 2024-10-15T13:54:02Z

internal/services/videoindexer/account_resource.go

+				if response.WasNotFound(account.HttpResponse) {
+					return metadata.MarkAsGone(id)
+				}
+				return fmt.Errorf("reading %s: %+v", id, err)


Suggested change

return fmt.Errorf("reading %s: %+v", id, err)

return fmt.Errorf("retrieving %s: %+v", id, err)

stephybun · 2024-10-15T13:55:31Z

internal/services/videoindexer/account_resource.go

+			id, err := accounts.ParseAccountID(metadata.ResourceData.Id())
+			if err != nil {
+				return err
+			}
+
+			client := metadata.Client.VideoIndexer.AccountClient
+			metadata.Logger.Infof("deleting %s", id)
+
+			if _, err := client.Delete(ctx, *id); err != nil {
+				return fmt.Errorf("deleting %s: %v", id, err)
+			}


Really minor, but the ordering of this looks strange to me. Don't we usually define the client first then begin parsing IDs?

Good shout! That does look weird

stephybun · 2024-10-15T13:56:46Z

internal/services/videoindexer/account_resource.go

+		StorageAccountId:       pointer.From(input.ResourceId),
+		UserAssignedIdentityId: pointer.From(input.UserAssignedIdentity),


We should parse the resource IDs that are returned by the API before setting them into state so that we can catch any casing inconsistencies

stephybun · 2024-10-15T13:58:57Z

internal/services/videoindexer/account_resource_test.go

+		if response.WasNotFound(resp.HttpResponse) {
+			return utils.Bool(false), nil
+		}
+		return nil, fmt.Errorf("retreiving %s: %v", id, err)
+	}
+	if response.WasNotFound(resp.HttpResponse) {
+		return utils.Bool(false), nil
+	}
+	return utils.Bool(true), nil


I'm not sure why we might need to specifically check for a 404 status, any error that we get from the API when running the tests and checking that it exists is logically a false, so I think this can be simplified. Let me know if that makes sense or if I've not considered something.

Suggested change

if response.WasNotFound(resp.HttpResponse) {

return utils.Bool(false), nil

}

return nil, fmt.Errorf("retreiving %s: %v", id, err)

}

if response.WasNotFound(resp.HttpResponse) {

return utils.Bool(false), nil

}

return utils.Bool(true), nil

return nil, fmt.Errorf("retrieving %s: %v", id, err)

}

return pointer.To(true), nil

Nah, you're right. Copy pasta strikes again

stephybun · 2024-10-15T13:59:17Z

internal/services/videoindexer/account_resource_test.go

+
+	data.ResourceTest(t, r, []acceptance.TestStep{
+		{
+			Config: r.basic(data),


Should this be using

Suggested change

Config: r.basic(data),

Config: r.userAssignedIdentity(data),

Ooo yep yep. Ty ty

stephybun

Thanks @mbfrahry LGTM 👍

github-actions · 2024-11-25T02:17:44Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

mbfrahry added 3 commits October 11, 2024 14:32

New Resource - azurerm_video_indexer_account

b2f11c9

Merge branch 'main' into f-video-indexer

690eec2

Add vendored files

63a7d6a

github-actions bot added documentation dependencies size/XL waiting-response labels Oct 11, 2024

Lint

648b592

github-actions bot added waiting-response and removed waiting-response labels Oct 11, 2024

website-lint

dca1814

github-actions bot added service/app-service waiting-response and removed waiting-response labels Oct 11, 2024

make generate

126c57c

github-actions bot removed the waiting-response label Oct 11, 2024

rcskosir added the new-resource label Oct 14, 2024

stephybun suggested changes Oct 15, 2024

View reviewed changes

github-actions bot added the waiting-response label Oct 15, 2024

address review

5ee3c51

github-actions bot added waiting-response and removed waiting-response labels Oct 15, 2024

mbfrahry force-pushed the f-video-indexer branch from 0671be2 to 5ee3c51 Compare October 17, 2024 21:12

mbfrahry requested review from a team and katbyte as code owners October 17, 2024 21:12

github-actions bot added waiting-response and removed waiting-response labels Oct 17, 2024

lint

67695d7

github-actions bot removed the waiting-response label Oct 17, 2024

stephybun approved these changes Oct 23, 2024

View reviewed changes

stephybun merged commit e3fd32d into main Oct 23, 2024

stephybun deleted the f-video-indexer branch October 23, 2024 13:32

github-actions bot added this to the v4.7.0 milestone Oct 23, 2024

stephybun added a commit that referenced this pull request Oct 23, 2024

Update for #27632

b77bd00

github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2024

	return fmt.Errorf("reading %s: %+v", id, err)
	return fmt.Errorf("retrieving %s: %+v", id, err)

		StorageAccountId: pointer.From(input.ResourceId),
		UserAssignedIdentityId: pointer.From(input.UserAssignedIdentity),

Conversation

mbfrahry commented Oct 11, 2024

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Uh oh!

stephybun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephybun left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants