From d416600ba592d5f74cf32030b3ffc72c430de7ed Mon Sep 17 00:00:00 2001 From: Chandler Swift Date: Fri, 5 Jan 2024 02:56:34 -0600 Subject: [PATCH] Linter: Ignore false positives in gradually-deprecated check (#24393) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/hashicorp/terraform-provider-azurerm/pull/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 [] [--merge-base] [--] [...] This form is to view the changes you have in your working tree relative to the named . 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 , use the merge base of 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 e7e6203436aa9d89a81eabd6171a72be146ced25 (HEAD) Author: Chandler Swift 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 Co-Authored-By: Ryan Wozney 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 --- scripts/run-gradually-deprecated.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/run-gradually-deprecated.sh b/scripts/run-gradually-deprecated.sh index 993814b35a82..f2a6b207858b 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