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

command/meta_backend: fix for cloud integration panic #30773

Merged
merged 1 commit into from Mar 31, 2022

Conversation

Uk1288
Copy link
Contributor

@Uk1288 Uk1288 commented Mar 30, 2022

Description

Currently if an error occurs during client configuration, b.Configure it gets swallowed. This PR returns the error after it gets added to error diagnostics.

Problem

Errors occurring during client configuration gets swallowed.

Fix

Fixes issue #30760

Tests

Unit tests already exists

Notes

Paired with @uturunku1

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 30, 2022

CLA assistant check
All committers have signed the CLA.

@Uk1288 Uk1288 changed the title fix for cloud integration panic command/meta_backend: fix for cloud integration panic Mar 30, 2022
@Uk1288 Uk1288 requested a review from alisdair March 30, 2022 22:11
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

🥳

@@ -320,6 +320,9 @@ func (m *Meta) BackendForPlan(settings plans.Backend) (backend.Enhanced, tfdiags

configureDiags := b.Configure(newVal)
diags = diags.Append(configureDiags)
if configureDiags.HasErrors() {
Copy link
Contributor

Choose a reason for hiding this comment

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

would write this check under the configureDiags assignment before appending. Besides that awesome work... I know a small change like this must have been painful to catch! 🔥

Copy link
Member

Choose a reason for hiding this comment

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

This is a tricky thing about how we handle diagnostics in Terraform. Because diags can contain warnings, we always append before checking for errors. This ensures that we always return diags (which may have built up a series of warnings before encountering an error), rather than writing if subDiags.HasErrors() { return subDiags } or similar.

All that to say: I think this change is actually okay, but wanted to point out that it is quite counter-intuitive.

Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

This looks good to me! I think it will also fix #30646.

@Uk1288 Uk1288 merged commit 1c9929a into main Mar 31, 2022
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@alisdair alisdair mentioned this pull request Mar 31, 2022
@Uk1288 Uk1288 mentioned this pull request Mar 31, 2022
@Uk1288 Uk1288 added cloud Related to Terraform Cloud's integration with Terraform 1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Mar 31, 2022
@github-actions
Copy link

github-actions bot commented May 1, 2022

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged cloud Related to Terraform Cloud's integration with Terraform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid memory address or nil pointer dereference
4 participants