-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
initwd: Port from earlyconfig
to configs
#32781
Conversation
This is a mostly mechanical refactor with a handful of changes which are necessary due to the semantic difference between earlyconfig and configs. When parsing root and descendant modules in the module installer, we now check the core version requirements inline. If the Terraform version is incompatible, we drop any other module loader diagnostics. This ensures that future language additions don't clutter the output and confuse the user. We also add two new checks during the module load process: * Don't try to load a module with a `nil` source address. This is a necessary change due to the move away from earlyconfig. * Don't try to load a module with a blank name (i.e. `module ""`). Because our module loading manifest uses the stringified module path as its map key, this causes a collision with the root module, and a later panic. This is the bug which triggered this refactor in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was interesting to review because so much of this code I haven't seen for several years at this point 🙃 so was cool to be reminded of some stuff we do in here that I'd forgotten about.
Of course it's tough to review a change like this in detail because my brain loves to pattern-match and subconsciously skim over things that look similar, but I did my best to read it over as closely as I could. I think though probably the best thing to do here is to merge this and let it soak in some alpha releases and other prereleases for a while to see if there are any unexpected edge-cases we haven't noticed with this yet.
This isn't currently used anywhere downstream, but it easily could be in the future, so populating the range with some zero-ish data is more robust.
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
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. |
This is a mostly mechanical refactor with a handful of changes which are necessary due to the semantic difference between
earlyconfig
andconfigs
.When parsing root and descendant modules in the module installer, we now check the core version requirements inline. If the Terraform version is incompatible, we drop any other module loader diagnostics. This ensures that future language additions don't clutter the output and confuse the user.
We also add two new checks during the module load process:
Don't try to load a module with a
nil
source address. This is a necessary change due to the move away fromearlyconfig
.Don't try to load a module with a blank name (i.e.
module ""
). Because our module loading manifest uses the stringified module path as its map key, this causes a collision with the root module, and a later panic. This is the bug which triggered this refactor in the first place.Target Release
1.5.x
Draft CHANGELOG entry
BUG FIXES
terraform init
: Fixed crash with invalid blank module name.