`azurerm_trusted_signing_account` - new resource support #27720

ziyeqf · 2024-10-22T04:11:14Z

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

Add new resource azurerm_trusted_signing_account per the document: https://learn.microsoft.com/en-us/azure/trusted-signing/concept-trusted-signing-resources-roles
The resource provider is named "Microsoft.Codesigning", however the name on Microsoft document is "Trusted signing", so I named this resource to `azurerm_trusted_signing_account"

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.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

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)

❯❯ tftest codesigning TestAccTrustedSigningAccount
=== RUN   TestAccTrustedSigningAccount_basic
=== PAUSE TestAccTrustedSigningAccount_basic
=== RUN   TestAccTrustedSigningAccount_update
=== PAUSE TestAccTrustedSigningAccount_update
=== RUN   TestAccTrustedSigningAccount_complete
=== PAUSE TestAccTrustedSigningAccount_complete
=== CONT  TestAccTrustedSigningAccount_basic
=== CONT  TestAccTrustedSigningAccount_complete
=== CONT  TestAccTrustedSigningAccount_update
--- PASS: TestAccTrustedSigningAccount_update (412.25s)
--- PASS: TestAccTrustedSigningAccount_complete (413.04s)
--- PASS: TestAccTrustedSigningAccount_basic (413.31s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/codesigning	413.386s

Change Log

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

azurerm_trusted_signing_account - new resource support [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 #26388

Note

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

magodo

Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

internal/services/codesigning/client/client.go

website/docs/r/trusted_signing_account.html.markdown

internal/services/codesigning/trusted_signing_account_resource_test.go

internal/services/codesigning/trusted_signing_account_resource.go

ziyeqf · 2024-10-23T05:48:40Z

test result after update

❯❯ tftest codesigning TestAccTrustedSigningAccount
=== RUN   TestAccTrustedSigningAccount_basic
=== PAUSE TestAccTrustedSigningAccount_basic
=== RUN   TestAccTrustedSigningAccount_update
=== PAUSE TestAccTrustedSigningAccount_update
=== RUN   TestAccTrustedSigningAccount_complete
=== PAUSE TestAccTrustedSigningAccount_complete
=== CONT  TestAccTrustedSigningAccount_basic
=== CONT  TestAccTrustedSigningAccount_complete
=== CONT  TestAccTrustedSigningAccount_update
--- PASS: TestAccTrustedSigningAccount_complete (487.05s)
--- PASS: TestAccTrustedSigningAccount_basic (490.52s)
--- PASS: TestAccTrustedSigningAccount_update (669.03s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/codesigning	669.104s

magodo · 2024-10-23T05:52:19Z

LGTM now, thank you!

stephybun

@ziyeqf please fix up the comments left in-line

stephybun · 2024-10-30T10:05:11Z

go.mod

+	github.com/hashicorp/go-azure-sdk/resource-manager v0.20241017.1093842
+	github.com/hashicorp/go-azure-sdk/sdk v0.20241017.1093842


Updating the go-azure-sdk should be done in a separate PR otherwise we end up causing conflicts like has happened here.

stephybun · 2024-10-30T10:07:48Z

internal/services/codesigning/trusted_signing_account_resource.go

+
+			var model TruestedSigningAccountModel
+			if err := meta.Decode(&model); err != nil {
+				return err


These errors should be wrapped

Suggested change

return err

return fmt.Errorf("decoding: %+v", err)

stephybun · 2024-10-30T10:10:07Z

internal/services/codesigning/trusted_signing_account_resource.go

+			req := codesigningaccounts.CodeSigningAccount{}
+			req.Name = &model.Name
+			req.Location = model.Location
+			req.Properties = pointer.To(codesigningaccounts.CodeSigningAccountProperties{})
+			req.Properties.Sku = pointer.To(codesigningaccounts.AccountSku{
+				Name: codesigningaccounts.SkuName(model.SkuName),
+			})
+			req.Tags = &model.Tags


Please update this to

Suggested change

req := codesigningaccounts.CodeSigningAccount{}

req.Name = &model.Name

req.Location = model.Location

req.Properties = pointer.To(codesigningaccounts.CodeSigningAccountProperties{})

req.Properties.Sku = pointer.To(codesigningaccounts.AccountSku{

Name: codesigningaccounts.SkuName(model.SkuName),

})

req.Tags = &model.Tags

req := codesigningaccounts.CodeSigningAccount{

Name: &model.Name

Location: location.Normalize(model.Location)

Tags: &model.Tags

Properties: &codesigningaccounts.CodeSigningAccountProperties{

Sku: &codesigningaccounts.AccountSku{

Name: codesigningaccounts.SkuName(model.SkuName),

}

}

}

stephybun · 2024-10-30T10:10:46Z

internal/services/codesigning/trusted_signing_account_resource.go

+			client := meta.Client.CodeSigning.Client.CodeSigningAccounts
+			result, err := client.Get(ctx, *id)
+			if err != nil {
+				return err


Suggested change

return err

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

stephybun · 2024-10-30T10:19:26Z

internal/services/codesigning/trusted_signing_account_resource.go

+			if result.Model == nil {
+				return fmt.Errorf("retrieving %s got nil model", id)
+			}
+			var output TruestedSigningAccountModel
+			output.Name = id.CodeSigningAccountName
+			output.ResourceGroupName = id.ResourceGroupName
+
+			if result.Model == nil {
+				return fmt.Errorf("Get response nil Model")
+			}
+			model := result.Model
+			output.Location = model.Location
+			output.Tags = pointer.From(model.Tags)
+			if ptrProp := model.Properties; ptrProp != nil {
+				itemProp := *ptrProp
+				output.AccountUri = pointer.From(itemProp.AccountUri)
+				if ptrSku := itemProp.Sku; ptrSku != nil {
+					itemSku := *ptrSku
+					output.SkuName = string(itemSku.Name)
+				}
+			}


We don't error if the model is nil in the read, yet we're doing it twice here. This is also inconsistent with our contributor guidelines and the rest of the provider. Please refactor this

Suggested change

if result.Model == nil {

return fmt.Errorf("retrieving %s got nil model", id)

}

var output TruestedSigningAccountModel

output.Name = id.CodeSigningAccountName

output.ResourceGroupName = id.ResourceGroupName

if result.Model == nil {

return fmt.Errorf("Get response nil Model")

}

model := result.Model

output.Location = model.Location

output.Tags = pointer.From(model.Tags)

if ptrProp := model.Properties; ptrProp != nil {

itemProp := *ptrProp

output.AccountUri = pointer.From(itemProp.AccountUri)

if ptrSku := itemProp.Sku; ptrSku != nil {

itemSku := *ptrSku

output.SkuName = string(itemSku.Name)

}

}

var output TruestedSigningAccountModel

output.Name = id.CodeSigningAccountName

output.ResourceGroupName = id.ResourceGroupName

if model := result.Model; model != nil {

output.Location = location.Normalize(model.Location)

output.Tags = pointer.From(model.Tags)

if props := model.Properties; props != nil {

output.AccountUri = pointer.From(props.AccountUri)

if sku := props.Sku; sku != nil {

output.SkuName = string(sku.Name)

}

}

}

stephybun · 2024-10-30T10:25:54Z

internal/services/codesigning/trusted_signing_account_resource_test.go

+	}
+	resp, err := client.CodeSigning.Client.CodeSigningAccounts.Get(ctx, *id)
+	if err != nil {
+		return nil, fmt.Errorf("retrieving TrustedSigningAccount %s: %+v", id, err)


Suggested change

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

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

stephybun · 2024-10-30T10:26:05Z

internal/services/codesigning/trusted_signing_account_resource_test.go

+	if err != nil {
+		return nil, fmt.Errorf("retrieving TrustedSigningAccount %s: %+v", id, err)
+	}
+	return utils.Bool(resp.Model != nil), nil


Suggested change

return utils.Bool(resp.Model != nil), nil

return pointer.To(resp.Model != nil), nil

stephybun · 2024-10-30T10:28:29Z

internal/services/codesigning/registration.go

+var _ sdk.TypedServiceRegistration = Registration{}
+
+func (r Registration) AssociatedGitHubLabel() string {
+	return "service/codesigning"


We should probably just call this

Suggested change

return "service/codesigning"

return "service/trustedsigning"

stephybun · 2024-10-30T10:29:15Z

website/docs/r/trusted_signing_account.html.markdown

+
+The following arguments are supported:
+
+* `name` - (Required) The name which should be used for this Trusted Signing. Changing this forces a new Trusted Signing Account to be created.


Please update all instances of Trusted Signing to Trusted Signing Account

Suggested change

* `name` - (Required) The name which should be used for this Trusted Signing. Changing this forces a new Trusted Signing Account to be created.

* `name` - (Required) The name which should be used for this Trusted Signing Account. Changing this forces a new Trusted Signing Account to be created.

stephybun · 2024-10-30T10:29:49Z

website/docs/r/trusted_signing_account.html.markdown

+
+* `id` - The ID of the Trusted Signing.
+
+* `account_uri` - The URI of the trusted signing account which is used during signing files.


Please be consistent with the casing throughout the documentation

Suggested change

* `account_uri` - The URI of the trusted signing account which is used during signing files.

* `account_uri` - The URI of the Trusted Signing Account which is used during signing files.

ziyeqf · 2024-10-31T03:46:37Z

I'm not sure why the CI failed for a change on .github/labeler-pull-request-triage.yml beyond this RP by make generate, seems not related.

and here is the new test result:

❯❯ tftest codesigning TestAccTrustedSigningAccount
=== RUN   TestAccTrustedSigningAccount_basic
=== PAUSE TestAccTrustedSigningAccount_basic
=== RUN   TestAccTrustedSigningAccount_update
=== PAUSE TestAccTrustedSigningAccount_update
=== RUN   TestAccTrustedSigningAccount_complete
=== PAUSE TestAccTrustedSigningAccount_complete
=== CONT  TestAccTrustedSigningAccount_basic
=== CONT  TestAccTrustedSigningAccount_complete
=== CONT  TestAccTrustedSigningAccount_update
--- PASS: TestAccTrustedSigningAccount_basic (412.82s)
--- PASS: TestAccTrustedSigningAccount_complete (413.86s)
--- PASS: TestAccTrustedSigningAccount_update (542.41s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/codesigning	542.497s

stephybun · 2024-10-31T10:04:02Z

internal/services/codesigning/trusted_signing_account_resource.go

+
+			req := codesigningaccounts.CodeSigningAccount{
+				Name:     &model.Name,
+				Location: model.Location,


Forgot to normalize this one as well, we normalize when building the payload as well as before setting into state

Suggested change

Location: model.Location,

Location: location.Normalize(model.Location),

stephybun

Thanks @ziyeqf LGTM 🍀

azurerm_trusted_signing_account - new resource support

0ad2225

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

make generate

7c0315a

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

magodo requested changes Oct 22, 2024

View reviewed changes

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

rcskosir added the new-resource label Oct 22, 2024

update per comments

041d9ba

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

ziyeqf marked this pull request as ready for review October 23, 2024 05:52

ziyeqf requested review from katbyte and a team as code owners October 23, 2024 05:52

stephybun requested changes Oct 30, 2024

View reviewed changes

ziyeqf added 2 commits October 31, 2024 10:52

Merge branch 'main' into code_sign

366dad6

update per comments

0e244d0

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

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

ziyeqf added 2 commits October 31, 2024 17:30

update code

d8a5afc

update typo and code pattern

3343a0d

stephybun reviewed Oct 31, 2024

View reviewed changes

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

normalize location

5421d7c

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

stephybun approved these changes Oct 31, 2024

View reviewed changes

stephybun merged commit 2d8b22f into hashicorp:main Oct 31, 2024
33 of 34 checks passed

github-actions bot added this to the v4.8.0 milestone Oct 31, 2024

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

Update for #27720

ecede1a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_trusted_signing_account` - new resource support #27720

`azurerm_trusted_signing_account` - new resource support #27720

ziyeqf commented Oct 22, 2024 •

edited

Loading

magodo left a comment

ziyeqf commented Oct 23, 2024

magodo commented Oct 23, 2024

stephybun left a comment

stephybun Oct 30, 2024

stephybun Oct 30, 2024

stephybun Oct 30, 2024

stephybun Oct 30, 2024

stephybun Oct 30, 2024

stephybun Oct 30, 2024 •

edited

Loading

stephybun Oct 30, 2024

stephybun Oct 30, 2024

stephybun Oct 30, 2024

stephybun Oct 30, 2024

ziyeqf commented Oct 31, 2024 •

edited

Loading

stephybun Oct 31, 2024

stephybun left a comment

		github.com/hashicorp/go-azure-sdk/resource-manager v0.20241017.1093842
		github.com/hashicorp/go-azure-sdk/sdk v0.20241017.1093842

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

	return utils.Bool(resp.Model != nil), nil
	return pointer.To(resp.Model != nil), nil


		The following arguments are supported:

		* `name` - (Required) The name which should be used for this Trusted Signing. Changing this forces a new Trusted Signing Account to be created.


		* `id` - The ID of the Trusted Signing.

		* `account_uri` - The URI of the trusted signing account which is used during signing files.

	Location: model.Location,
	Location: location.Normalize(model.Location),

azurerm_trusted_signing_account - new resource support #27720

azurerm_trusted_signing_account - new resource support #27720

Conversation

ziyeqf commented Oct 22, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

magodo left a comment

Choose a reason for hiding this comment

ziyeqf commented Oct 23, 2024

magodo commented Oct 23, 2024

stephybun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephybun Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyeqf commented Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

`azurerm_trusted_signing_account` - new resource support #27720

`azurerm_trusted_signing_account` - new resource support #27720

ziyeqf commented Oct 22, 2024 •

edited

Loading

stephybun Oct 30, 2024 •

edited

Loading

ziyeqf commented Oct 31, 2024 •

edited

Loading