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

Restore azurerm_nginx_configuration resource and data source #25773

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

valyria257
Copy link
Contributor

Reverts #24276

There are a couple of things here:

  • The default configuration/import error issue was resolved. See changelog from March 13, 2024
  • This is a breaking change from a product use-case perspective as it breaks our HTTPS use-case where in the workflow is:
    1. Create deployment.
    2. Create certificate.
    3. Create configuration.

With configuration now being inline, the use workflow breaks as 1 and 3 cannot be separated.

  • certificates should also not be inline since different roles may mange deployments, certificates, and configurations separately

To fix this, the change to remove the azurerm_nginx_configuration is reverted, and the field that was added to replace it is deprecated.

"tags": commonschema.Tags(),
}

if !features.FourPointOhBeta() {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this how features are deprecated in TF? I have not looked at this function but I vaguely remember that this was for breaking changes when the provider gets a 4.0 bumped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I got that from this contributing guide: https://github.com/hashicorp/terraform-provider-azurerm/blob/main/contributing/topics/guide-new-fields-to-resource.md#renaming-and-deprecating-a-property, but could use some direction here if there's a different way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for sharing this 👍

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @valyria257! Overall this looks fine, the tests are running. I left a few very minor comments around consistency and formatting, would you mind fixing those up? Once that's done and the tests are green this should be good to go!

internal/services/nginx/nginx_deployment_resource.go Outdated Show resolved Hide resolved
internal/services/nginx/nginx_deployment_resource.go Outdated Show resolved Hide resolved
website/docs/r/nginx_configuration.html.markdown Outdated Show resolved Hide resolved
website/docs/r/nginx_configuration.html.markdown Outdated Show resolved Hide resolved
website/docs/r/nginx_configuration.html.markdown Outdated Show resolved Hide resolved
website/docs/r/nginx_configuration.html.markdown Outdated Show resolved Hide resolved
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @valyria257. Quite a lot of the tests have been failing for a while with the same error message which prevents us from validating the changes made in this PR. Would you be able to take a look at this?

------- Stdout: -------
=== RUN   TestAccConfiguration_basic
=== PAUSE TestAccConfiguration_basic
=== CONT  TestAccConfiguration_basic
    testcase.go:113: Step 1/2 error: Error running apply: exit status 1
        Error: creating Nginx Deployment (Subscription: "*******"
        Resource Group Name: "acctestRG-auto-240530144011976922"
        Nginx Deployment Name: "acctest-240530144011976922"): polling after DeploymentsCreateOrUpdate: polling failed: the Azure API returned the following error:
        Status: "Failed"
        Code: "MissingManagedIdentity"
        Message: "The NGINXaaS deployment is missing a managed identity. Add a managed identity and try again. If you want to contact F5 NGINX support please open a request at https://my.f5.com/manage/s/
        Activity Id: ""

The NGINXaaS API now requires a managed identity to be added to the depoyment if metrics are enabled.
This updates tests which do not have an identity to set diagnose_support_enabled to false.
Now that the NGINXaaS API does not provide a default config, we need to ensure the resource gets created
before referencing the data source, so we don't get config not found errors.
@valyria257
Copy link
Contributor Author

@stephybun the nginx acceptance tests should now all pass. Please let me know if you see any other errors, thanks!

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @valyria257 LGTM 👍

@stephybun stephybun merged commit 4508704 into hashicorp:main Jun 4, 2024
33 checks passed
@github-actions github-actions bot added this to the v3.107.0 milestone Jun 4, 2024
stephybun added a commit that referenced this pull request Jun 6, 2024
dduportal added a commit to jenkins-infra/azure that referenced this pull request Jun 11, 2024
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/azurerm&#34; updated from
&#34;3.104.2&#34; to &#34;3.107.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.107.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.107.0&#xA;FEATURES:&#xA;&#xA;*
**New Resource:**
`azurerm_data_protection_backup_policy_postgresql_flexible_server`
([#26024](https://github.com/hashicorp/terraform-provider-azurerm/issues/26024))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.20240604.1114748` of
`github.com/hashicorp/go-azure-sdk`
([#26216](https://github.com/hashicorp/terraform-provider-azurerm/issues/26216))&#xA;*
`advisor`: update API version to `2023-01-01`
([#26205](https://github.com/hashicorp/terraform-provider-azurerm/issues/26205))&#xA;*
`keyvault`: handling the Resources API returning Key Vaults that have
been deleted when populating the cache
([#26199](https://github.com/hashicorp/terraform-provider-azurerm/issues/26199))&#xA;*
`machinelearning`: update API version to `2024-04-01`
([#26168](https://github.com/hashicorp/terraform-provider-azurerm/issues/26168))&#xA;*
`network/privatelinkservices` - update to use `hashicorp/go-azure-sdk`
([#26212](https://github.com/hashicorp/terraform-provider-azurerm/issues/26212))&#xA;*
`network/serviceendpointpolicies` - update to use
`hashicorp/go-azure-sdk`
([#26196](https://github.com/hashicorp/terraform-provider-azurerm/issues/26196))&#xA;*
`network/virtualnetworks` - update to use `hashicorp/go-azure-sdk`
([#26217](https://github.com/hashicorp/terraform-provider-azurerm/issues/26217))&#xA;*
`network/virtualwans`: update route resources to use
`hashicorp/go-azure-sdk`
([#26189](https://github.com/hashicorp/terraform-provider-azurerm/issues/26189))&#xA;*
`azurerm_container_app_job` - support for the `key_vault_secret_id` and
`identity` properties in the `secret` block
([#25969](https://github.com/hashicorp/terraform-provider-azurerm/issues/25969))&#xA;*
`azurerm_kubernetes_cluster` - support forthe `dns_zone_ids` popperty in
the `web_app_routing` block
([#26117](https://github.com/hashicorp/terraform-provider-azurerm/issues/26117))&#xA;*
`azurerm_notification_hub_authorization_rule` - support for the
`primary_connection_string` and `secondary_connection_string` properties
([#26188](https://github.com/hashicorp/terraform-provider-azurerm/issues/26188))&#xA;*
`azurerm_subnet` - support for the `default_outbound_access_enabled`
property
([#25259](https://github.com/hashicorp/terraform-provider-azurerm/issues/25259))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_api_management_named_value` - will now
enforce setting the `secret` property when setting the
`value_from_key_vault` property
([#26150](https://github.com/hashicorp/terraform-provider-azurerm/issues/26150))&#xA;*
`azurerm_storage_sync_server_endpoint` - improve pooling to work around
api inconsistencies
([#26204](https://github.com/hashicorp/terraform-provider-azurerm/issues/26204))&#xA;*
`azurerm_virtual_network` - split create and update function to fix
lifecycle - ignore
([#26246](https://github.com/hashicorp/terraform-provider-azurerm/issues/26246))&#xA;*
`azurerm_vpn_server_configuration` - split create and update function to
fix lifecycle - ignore
([#26175](https://github.com/hashicorp/terraform-provider-azurerm/issues/26175))&#xA;*
`azurerm_vpn_server_configuration_policy_group` - split create and
update function to fix lifecycle - ignore
([#26207](https://github.com/hashicorp/terraform-provider-azurerm/issues/26207))&#xA;*
`azurerm_vpn_site` - split create and update function to fix lifecycle -
ignore changes
([#26163](https://github.com/hashicorp/terraform-provider-azurerm/issues/26163))&#xA;&#xA;DEPRECATIONS:&#xA;&#xA;*
`azurerm_kubernetes_cluster` - the property `dns_zone_id` has been
superseded by the property `dns_zone_ids` in the `web_app_routing` block
([#26117](https://github.com/hashicorp/terraform-provider-azurerm/issues/26117))&#xA;*
`azurerm_nginx_deployment` - the block `configuration` has been
deprecated and superseded by the resource `azurerm_nginx_configuration`
([#25773](https://github.com/hashicorp/terraform-provider-azurerm/issues/25773))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/229/">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>

---------

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
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

3 participants