provider: log instead of error when RPs are unavailable when validating RP registrations #23380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n validating RP registrations
Comment on lines
26
to
29
| if environment.Name == environments.AzurePublicCloud { | ||
| // this is likely a typo in the Required Resource Providers list, so we should surface this | ||
| return nil, fmt.Errorf("the required Resource Provider %q wasn't returned from the Azure API", providerName) | ||
| } |
There was a problem hiding this comment.
thinking about this we should probably remove this error here, and just log all the time - the unit test should fail but if an RP gets removed we shouldn't flag an error here?
|
Is there any way to push this PR further? We are currently blocked by the issue that this PR should solve. |
mbfrahry
approved these changes
Nov 2, 2023
mbfrahry
changed the title
mbfrahry
deleted the
bugfix/rp-registration-cache-workaround-for-non-public
branch
mbfrahry
added a commit
that referenced
this pull request
Nov 2, 2023
dduportal
pushed a commit
to jenkins-infra/azure
that referenced
this pull request
Nov 6, 2023
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
<h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
<summary>Update Terraform lock file</summary>
<p>"hashicorp/azurerm" updated from "3.77.0" to
"3.79.0" in file ".terraform.lock.hcl"</p>
<details>
<summary>3.79.0</summary>
<pre>Changelog retrieved
from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.79.0
ENHANCEMENTS:

*
provider: log instead of error when RPs are unavailable when validating
RP registrations
([#23380](hashicorp/terraform-provider-azurerm#23380
`azurerm_arc_kuberenetes_cluster_extension_resource` - the `version` and
`release_train` properties can now be set simultaneously
([#23692](hashicorp/terraform-provider-azurerm#23692
`azurerm_container_apps` - support for the `ingress.exposed_port`
property
([#23752](hashicorp/terraform-provider-azurerm#23752
`azurerm_cosmosdb_postgresql_cluster` - read replica clusters can be
created without specifying `administrator_login_password` property
([#23750](hashicorp/terraform-provider-azurerm#23750
`azurerm_managed_application` - arrays can be supplied in the
`parameter_values` property
([#23754](hashicorp/terraform-provider-azurerm#23754
`azurerm_storage_management_policy` - support for properties
`rule.*.actions.*.base_blob.0.tier_to_cold_after_days_since_{modification|last_access_time|creation}_greater_than
and
rule.*.actions.*.{snapshot|version}.0.tier_to_cold_after_days_since_creation_greater_than`
([#23574](https://github.com/hashicorp/terraform-provider-azurerm/issues/23574))

BUG
FIXES:

* `azurerm_api_management_diagnostic` - the
`operation_name_format` attribute will only be sent if `identifier` is
set to `applicationinsights`
([#23736](hashicorp/terraform-provider-azurerm#23736
`azurerm_backup_policy_vm` - fix payload by using current datetime
([#23586](hashicorp/terraform-provider-azurerm#23586
`azurerm_kubernetes_cluster` - the `custom_ca_trust_certificates_base64`
property can not be removed, only updated
([#23737](https://github.com/hashicorp/terraform-provider-azurerm/issues/23737))


</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) <60776566+jenkins-infra-bot@users.noreply.github.com>
katbyte
pushed a commit
that referenced
this pull request
Jan 5, 2024
Before this commit, the `scripts/run-gradually-deprecated.sh` check took the list of files that differed between `HEAD` (often the tip of a PR branch) and the current tip of `origin/main`, and checked `HEAD`'s version for use of a set of idioms marked `deprecated`. Unfortunately, this is pretty prone to false positives, where the use of a deprecated idiom is fixed in `origin/main` but not yet included in `HEAD`; making the check fail, as the deprecated use is present in `HEAD`, although it would not be present post-merge, as `HEAD` would be merged on top of the existing fix in `origin/main`. Specifically, I ran into this problem three times in a row (and counting!) on another PR, which is my motivation for this fix: #24071 Those all followed the pattern mentioned above: 1. I make a commit, containing no deprecated uses; merge gating checks don't yet run because they haven't been approved 2. Some deprecated use is fixed on main 3. The merge checks are approved by a maintainer, run, and fail because of step 2 4. I see that something's changed and rebase on the new changes; `goto` step 1. From the `git` man page: git diff [<options>] [--merge-base] <commit> [--] [<path>...] This form is to view the changes you have in your working tree relative to the named <commit>. You can use HEAD to compare it with the latest commit, or a branch name to compare with the tip of a different branch. If --merge-base is given, instead of using <commit>, use the merge base of <commit> and HEAD. git diff --merge-base A is equivalent to git diff $(git merge-base A HEAD). So in essence, this causes the script to only check changes from this PR rather than this PR's changes plus the inverse of every other commit to `main`. This other change would also be equivalent, but is in my opinion less clear as to what it does: ```diff diff --git a/scripts/run-gradually-deprecated.sh b/scripts/run-gradually-deprecated.sh index 993814b35a..f2a6b20785 100755 --- a/scripts/run-gradually-deprecated.sh +++ b/scripts/run-gradually-deprecated.sh @@ -9 +9 @@ function runGraduallyDeprecatedFunctions { - IFS=$'\n' read -r -d '' -a flist < <(git diff --diff-filter=AMRC origin/main --name-only) + IFS=$'\n' read -r -d '' -a flist < <(git diff --diff-filter=AMRC origin/main... --name-only) ``` Here's the testing I did, that confirms this would behave the way I expect: ``` % git show --stat # Here I'm on the commit that caused me problems in the above-mentioned PR commit e7e6203 (HEAD) Author: Chandler Swift <chandler+pearson@chandlerswift.com> Date: Wed Nov 29 12:26:20 2023 -0600 provider: don't attempt to register unavailable RPs PR #23380 addressed issue #21785, but with that fix, when an unavailable resource provider was found, it would still be added to the list of providers to be registered, and then fail upon attempted registration. This skips registration for providers which are neither registered nor unregistered. Co-Authored-By: Jeff Smith <toxicglados@gmail.com> Co-Authored-By: Ryan Wozney <wozneyr@users.noreply.github.com> internal/resourceproviders/requiring_registration.go | 1 + 1 file changed, 1 insertion(+) % git diff --diff-filter=AMRC origin/main --name-only | head # The diff shows many files CHANGELOG.md go.mod go.sum internal/resourceproviders/requiring_registration.go internal/services/authorization/role_definition_data_source.go internal/services/authorization/role_definition_data_source_test.go internal/services/automation/automation_module_resource.go internal/services/automation/automation_runbook_resource.go internal/services/automation/automation_runbook_resource_test.go internal/services/automation/registration.go % git diff --diff-filter=AMRC origin/main --name-only | wc -l # _maaaaany_ files 480 % git diff --diff-filter=AMRC origin/main --name-only --merge-base # but with `--merge-base`, just the one we expect internal/resourceproviders/requiring_registration.go % ./scripts/run-gradually-deprecated.sh # And sure enough, the unmodified script fails on an unrelated file: ==> Checking for use of gradually deprecated functions... internal/services/iothub/iothub_resource.go:73: Create: resourceIotHubCreateUpdate, New Resources should no longer use combined CreateUpdate methods, please split these into two separate Create and Update methods. Existing resources can continue to use combined CreateUpdate methods for the moment - but over time these will be split into separate Create and Update methods. % vim ./scripts/run-gradually-deprecated.sh # add `--merge-base` % git diff diff --git a/scripts/run-gradually-deprecated.sh b/scripts/run-gradually-deprecated.sh index 993814b35a..f2a6b20785 100755 --- a/scripts/run-gradually-deprecated.sh +++ b/scripts/run-gradually-deprecated.sh @@ -6,7 +6,7 @@ function runGraduallyDeprecatedFunctions { echo "==> Checking for use of gradually deprecated functions..." - IFS=$'\n' read -r -d '' -a flist < <(git diff --diff-filter=AMRC origin/main --name-only) + IFS=$'\n' read -r -d '' -a flist < <(git diff --diff-filter=AMRC origin/main --name-only --merge-base) for f in "${flist[@]}"; do # require resources to be imported is now hard-coded on - but only checking for additions % ./scripts/run-gradually-deprecated.sh # Now it doesn't false-positive! ==> Checking for use of gradually deprecated functions... ==> Checking for use of deprecated functions... % # Here I add a line that _should_ cause a failure: % echo features.ShouldResourcesBeImported >> internal/resourceproviders/requiring_registration.go % git diff internal/resourceproviders/requiring_registration.go diff --git a/internal/resourceproviders/requiring_registration.go b/internal/resourceproviders/requiring_registration.go index 65c0f08bf1..b6e8927100 100644 --- a/internal/resourceproviders/requiring_registration.go +++ b/internal/resourceproviders/requiring_registration.go @@ -31,3 +31,4 @@ func DetermineWhichRequiredResourceProvidersRequireRegistration(requiredResource return &requiringRegistration, nil } +features.ShouldResourcesBeImported % ./scripts/run-gradually-deprecated.sh # ...which, indeed, it does! ==> Checking for use of gradually deprecated functions... internal/resourceproviders/requiring_registration.go:34:features.ShouldResourcesBeImported The Feature Flag for 'ShouldResourcesBeImported' will be deprecated in the future and shouldn't be used in new resources - please remove new usages of the 'ShouldResourcesBeImported' function from these changes - since this is now enabled by default. In the future this function will be marked as Deprecated - however it's not for the moment to not conflict with open Pull Requests. ``` Co-authored-by: Jeff Smith <toxicglados@gmail.com>
stephybun
pushed a commit
that referenced
this pull request
Jan 25, 2024
PR #23380 addressed issue #21785, but with that fix, when an unavailable resource provider was found, it would still be added to the list of providers to be registered, and then fail upon attempted registration. This skips registration for providers which are neither registered nor unregistered. Co-authored-by: Jeff Smith <toxicglados@gmail.com> Co-authored-by: Ryan Wozney <wozneyr@users.noreply.github.com>
rizkybiz
pushed a commit
to rizkybiz/terraform-provider-azurerm
that referenced
this pull request
Jan 30, 2024
PR hashicorp#23380 addressed issue hashicorp#21785, but with that fix, when an unavailable resource provider was found, it would still be added to the list of providers to be registered, and then fail upon attempted registration. This skips registration for providers which are neither registered nor unregistered. Co-authored-by: Jeff Smith <toxicglados@gmail.com> Co-authored-by: Ryan Wozney <wozneyr@users.noreply.github.com>
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #21785