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

Typed SDK and pluginsdk wrapper #1188

Merged
merged 10 commits into from
Oct 16, 2023
Merged

Typed SDK and pluginsdk wrapper #1188

merged 10 commits into from
Oct 16, 2023

Conversation

manicminer
Copy link
Contributor

@manicminer manicminer commented Sep 13, 2023

  • Bump all dependencies including github.com/hashicorp/terraform-plugin-sdk/v2
  • Add Typed SDK
  • Add pluginsdk wrapper package
  • Update acceptance framework
  • Use terraform-plugin-testing module
  • Convert data.azuread_client_config and data.azuread_domains to typed data source
  • Convert azuread_directory_role to typed resource

To aid review, here's a rough breakdown of the changes by package:

Package/File Ported from AzureRM Notes
internal/acceptance Partially Merged with AzureRM package as there are local additions to help with AAD constructs
internal/acceptance/plugin_sdk_aliases.go Yes
internal/helpers - Use pluginsdk wrappers
internal/helpers/tf Yes From helpers/tf
internal/provider - Support typed resources
internal/sdk Yes -
internal/services/... - Use pluginsdk and acceptance wrappers, also validation refactoring
internal/services/domains - Port data.azuread_domains to typed SDK
internal/services/serviceprincipals - Port data.azuread_client_config to typed SDK
internal/tf/import.go - Removed, boilerplate
internal/tf/pluginsdk Yes Addition of wrappers for diag package
internal/tf/validation/pluginsdk.go Yes -
internal/validate - Moved to internal/tf/validation

…tance framework, convert data.azuread_domains to typed data source
@manicminer manicminer requested a review from a team September 13, 2023 01:21
@manicminer manicminer force-pushed the refactor/typed-sdk branch 2 times, most recently from 2355ee6 to a02dcaf Compare September 13, 2023 13:19
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Left some comments inline, but if we can fix those up then this should otherwise be good to go 👍

internal/acceptance/testcase.go Outdated Show resolved Hide resolved
internal/acceptance/testcase.go Outdated Show resolved Hide resolved
azurerm := provider.AzureADProvider()
return azurerm, nil
},
"azurerm-alt": func() (*schema.Provider, error) { //nolint:unparam
Copy link
Contributor

Choose a reason for hiding this comment

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

although does AAD need both provider instances?

Suggested change
"azurerm-alt": func() (*schema.Provider, error) { //nolint:unparam
"azuread-alt": func() (*schema.Provider, error) { //nolint:unparam

Copy link
Contributor Author

@manicminer manicminer Oct 16, 2023

Choose a reason for hiding this comment

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

Gonna remove the alt instance for now, we'll need 1-2 alt providers in time, but those can be added later

internal/acceptance/testcase.go Outdated Show resolved Hide resolved
internal/acceptance/testcase.go Outdated Show resolved Hide resolved
internal/sdk/README.md Outdated Show resolved Hide resolved
internal/sdk/README.md Outdated Show resolved Hide resolved
Optional: true,
Computed: true,
ValidateDiagFunc: validate.NoEmptyStrings,
ValidateDiagFunc: validation.ValidateDiag(validation.StringIsNotEmpty),
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than wrapping this, why not make the StringIsNotEmpty functions return diags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed, wrapping these will help when migrating back to error-based validation funcs

internal/services/domains/domains_data_source.go Outdated Show resolved Hide resolved
internal/services/domains/domains_data_source.go Outdated Show resolved Hide resolved
@manicminer
Copy link
Contributor Author

Test results

Screenshot 2023-10-16 at 23 09 32 Screenshot 2023-10-16 at 23 09 15 Screenshot 2023-10-16 at 23 09 05

@manicminer manicminer added this to the v2.44.0 milestone Oct 16, 2023
@manicminer manicminer merged commit 520c3eb into main Oct 16, 2023
15 checks passed
@manicminer manicminer deleted the refactor/typed-sdk branch October 16, 2023 22:12
manicminer added a commit that referenced this pull request Oct 16, 2023
dduportal referenced this pull request in jenkins-infra/azure Oct 25, 2023
<Actions>
<action
id="c2aadc6326b4b0bc58df11ee286b0f67ccdb5888bd77f391e6473570113337ec">
        <h3>Bump Terraform `azuread` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azuread&#34; updated from &#34;2.43.0&#34; to
&#34;2.44.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>2.44.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azuread/releases/tag/v2.44.0&#xA;*
Developer Note: the Typed Resource SDK, as also used in the AzureRM
provider, is now the preferred way of introducing new resources
([#1188](https://github.com/hashicorp/terraform-provider-azuread/issues/1188))&#xA;&#xA;FEATURES:&#xA;&#xA;*
**New Resource:** `azuread_application_api_access`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_application_app_role`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_application_fallback_public_client`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_application_from_template`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_application_identifier_uri`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_application_known_clients`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_application_owner`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_application_permission_scope
([#1214](https://github.com/hashicorp/terraform-provider-azuread/issues/1214))`&#xA;*
**New Resource:** `azuread_application_redirect_uris`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_application_registration`
([#1214](hashicorp/terraform-provider-azuread#1214
**New Resource:** `azuread_authentication_strength_policy`
([#1171](https://github.com/hashicorp/terraform-provider-azuread/issues/1171))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
`data.azuread_application` - export the `client_id` attribute, deprecate
the `application_id` attribute
([#1214](hashicorp/terraform-provider-azuread#1214
`data.azuread_service_principal` - support for the `client_id` property,
deprecate the `application_id` property
([#1214](hashicorp/terraform-provider-azuread#1214
`data.azuread_service_principals` - support for the `client_ids`
property, deprecate the `application_ids` property
([#1214](hashicorp/terraform-provider-azuread#1214
`data.azuread_service_principals` - export the `client_id` attribute in
the `service_principals` block, deprecate the `application_id` attribute
([#1214](hashicorp/terraform-provider-azuread#1214
`azuread_application` - export the `client_id` attribute, deprecate the
`application_id` attribute
([#1214](hashicorp/terraform-provider-azuread#1214
`azuread_application_federated_identity_credential` - support for the
`application_id` property, deprecate the `application_object_id`
property
([#1214](hashicorp/terraform-provider-azuread#1214
`azuread_application_certificate` - support for the `application_id`
property, deprecate the `application_object_id` property
([#1214](hashicorp/terraform-provider-azuread#1214
`azuread_application_password` - support for the `application_id`
property, deprecate the `application_object_id` property
([#1214](hashicorp/terraform-provider-azuread#1214
`azuread_application_pre_authorized` - support for the `application_id`
property, deprecate the `application_object_id` property
([#1214](hashicorp/terraform-provider-azuread#1214
`azuread_service_principal` - support for the `client_id` property,
deprecate the `application_id` property
([#1214](hashicorp/terraform-provider-azuread#1214
`azuread_conditional_access_policy` - support for the
`authentication_strength_policy_id` property in the `grant_controls`
block [GH_1171]&#xA;&#xA;BUG FIXES:&#xA;&#xA;* `azuread_group_member` -
resolve a bug when refreshing state if the group is missing
([#1198](https://github.com/hashicorp/terraform-provider-azuread/issues/1198))&#xA;&#xA;&#xA;</pre>
            </details>
            <details>
                <summary>2.44.1</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azuread/releases/tag/v2.44.1&#xA;BUG
FIXES:&#xA;&#xA;* `azuread_application_certificate` - work around an
unexpected diff with the `application_object_id` property
([#1221](hashicorp/terraform-provider-azuread#1221
`azuread_application_federated_identity_credential` - work around an
unexpected diff with the `application_object_id` property
([#1221](hashicorp/terraform-provider-azuread#1221
`azuread_application_password` - work around an unexpected diff with the
`application_object_id` property
([#1221](hashicorp/terraform-provider-azuread#1221
`azuread_application_pre_authorized` - work around an unexpected diff
with the `application_object_id` property
([#1221](https://github.com/hashicorp/terraform-provider-azuread/issues/1221))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

---------

Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants