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

Fix to issue: 1296 azuread_application_pre_authorized does not properly destroy #1299

Conversation

KenSpur
Copy link
Contributor

@KenSpur KenSpur commented Jan 26, 2024

@KenSpur KenSpur marked this pull request as ready for review January 26, 2024 12:14
@KenSpur
Copy link
Contributor Author

KenSpur commented Jan 26, 2024

Ready for review!

Is .github/labeler-pull-request-triage.yaml something i should update?

…stroy

Co-authored-by: KenSpur <spur.ken@gmail.com>
@manicminer manicminer force-pushed the 1296-azuread-application-pre-authorized-destroy-issue branch from 799d815 to 1818f0a Compare May 8, 2024 22:11
Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@KenSpur Thank you for submitting this PR, and apologies for the delay in reviewing.

We generally try to avoid using artificial delays as a way to combat concurrency or consistency issues. We have a locking mechanism in the provider which is intended to cover this kind of problem with parallel updates to the same underlying resource or object (i.e. an application in this case). This is present in the Create and Update functions for this resource, but it looks like I missed adding it to the Delete function.

tf.LockByName(applicationResourceName, id.ObjectId)
defer tf.UnlockByName(applicationResourceName, id.ObjectId)

Due to the tardiness of this review, I hope you don't mind but I've reworked this change using this approach, and added some checks to the acceptance test.

Thanks again for spotting this bug and working on the fix!

@manicminer
Copy link
Member

Test results

Screenshot 2024-05-08 at 23 17 12

@manicminer manicminer added the bug label May 8, 2024
@manicminer manicminer merged commit 2f696eb into hashicorp:main May 8, 2024
25 checks passed
@github-actions github-actions bot added this to the v2.49.0 milestone May 8, 2024
manicminer added a commit that referenced this pull request May 8, 2024
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request May 10, 2024
<Actions>
<action
id="6d17e7acdb2f3311576150379e22805f2f9b4aa72ff00ec136aceee45cae4b98">
        <h3>Bump Terraform `azuread` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/azuread&#34; updated from
&#34;2.48.0&#34; to &#34;2.49.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>2.49.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azuread/releases/tag/v2.49.0&#xA;FEATURES:&#xA;&#xA;*
**New Data Source:** `azuread_group_role_management_policy`
([#1327](https://github.com/hashicorp/terraform-provider-azuread/issues/1327))&#xA;*
**New Resource:** `azuread_group_role_management_policy`
([#1327](https://github.com/hashicorp/terraform-provider-azuread/issues/1327))&#xA;*
**New Resource:** `azuread_privileged_access_group_assignment_schedule`
([#1327](https://github.com/hashicorp/terraform-provider-azuread/issues/1327))&#xA;*
**New Resource:** `azuread_privileged_access_group_eligibility_schedule`
([#1327](https://github.com/hashicorp/terraform-provider-azuread/issues/1327))&#xA;*
**New Resource:** `azuread_synchronization_job_provision_on_demand`
([#1032](https://github.com/hashicorp/terraform-provider-azuread/issues/1032))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
`data.azuread_group` - support for the `include_transitive_members`
property
([#1300](https://github.com/hashicorp/terraform-provider-azuread/issues/1300))&#xA;*
`azuread_application` - relax validation for the `identifier_uris`
property to allow more values
([#1351](https://github.com/hashicorp/terraform-provider-azuread/issues/1351))&#xA;*
`azuread_application_identifier_uri` - relax validation for the
`identifier_uri` property to allow more values
([#1351](https://github.com/hashicorp/terraform-provider-azuread/issues/1351))&#xA;*
`azuread_group` - support the `SkipExchangeInstantOn` value for the
`behaviors` property
([#1370](https://github.com/hashicorp/terraform-provider-azuread/issues/1370))&#xA;*
`azuread_user` - relax validation for the `employee_type` property to
allow more values
([#1328](https://github.com/hashicorp/terraform-provider-azuread/issues/1328))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azuread_application_pre_authorized` - fix a
destroy-time bug that could prevent deletion of the resource
([#1299](https://github.com/hashicorp/terraform-provider-azuread/issues/1299))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/158/">Jenkins
pipeline link</a>
    </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) <60776566+jenkins-infra-bot@users.noreply.github.com>
@KenSpur KenSpur deleted the 1296-azuread-application-pre-authorized-destroy-issue branch May 15, 2024 07:36
BrendanThompson pushed a commit to BrendanThompson/terraform-provider-azuread that referenced this pull request Jun 18, 2024
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.

None yet

2 participants