[pull] main from hashicorp:main #155
Closed
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
![pull[bot]](https://avatars.githubusercontent.com/in/12910?s=60&v=4){kind=small}
…signment` resource type (#24365) * fix managed hsm role assignment document resource type * update description of fields
* `azurerm_iothub_device-update_account`- `sku` forces new now * remove sku test
…_filter setting (#24306) Co-authored-by: Oleksii Sheludchenko <[email protected]>
…when running Hotpatch-compatible images (#24335)
… for 4.0 and rename `enable_*` to `*_enabled` (#24167) * remove node_taints in v4.0 * rename enable_* to *_enabled * goimports
* `azurerm_iothub` - split create and udpate function * add cosmosdb endpoint test case * update per comment * revert mistake edit
…tection_backup_policy_disk`: support new property `time_zone` (#24312)
…ock (#24314) * add `identity` to datasource `azurerm_kusto_cluster` * rename identity to identityMap * Fixes imports sequences
…il pointer error (#24315)
…`tags` property (#24333)
github-actions
bot
added
documentation
The current version of documentation has colons on the Note section on the line after the header making the sentences look off.
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 <[email protected]> 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 <[email protected]> Co-Authored-By: Ryan Wozney <[email protected]> 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 <[email protected]>
|
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.
See Commits and Changes for more details.
Created by
pull[bot]
Can you help keep this open source service alive? 💖 Please sponsor : )