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

fix: skip check evaluation when context is not prepared successfully #35408

Merged

Conversation

burnerlee
Copy link
Contributor

@burnerlee burnerlee commented Jul 1, 2024

This PR fixes the terraform test cmd logging additional errorenous error message while referencing invalid attributes. While evaluating a check, a HCL evaluation context is created for the test command. Some validation is done during the creation of the context, failing which an empty context is returned with the expected diagnostic. This PR stops the evaluation of a check as soon as a diagnostic is returned with an error.

Fixes #35265

Target Release

1.9.1

Draft CHANGELOG entry

BUG FIXES

  • The terraform test command is expected to not show confusing message when context does not contain variables due to validation error
  • The expected behaviour is shown for the case mentioned in the issue description:
image

Copy link

hashicorp-cla-app bot commented Jul 1, 2024

CLA assistant check
All committers have signed the CLA.

@burnerlee burnerlee force-pushed the burnerlee/fix-terraform-test branch from 07b3bed to c66d40d Compare July 1, 2024 18:09
@burnerlee
Copy link
Contributor Author

@liamcervante could you please review?

@liamcervante liamcervante self-requested a review July 1, 2024 19:45
@liamcervante liamcervante added the 1.9-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Jul 2, 2024
Copy link
Member

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the contribution @burnerlee!

@liamcervante liamcervante merged commit 874483f into hashicorp:v1.8 Jul 2, 2024
6 checks passed
Copy link

github-actions bot commented Jul 2, 2024

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

@liamcervante
Copy link
Member

Whoops, I missed this was being merged directly into the v1.8 branch. We should have merged this into main and then let the backport bot automatically push into the most recent release branch. I should have checked the target branch more carefully.

I've reverted this PR in #35410, and merged this into the correct branch in #35411. @burnerlee, the commit that lands in main should still have your Github profile associated with it 👍

liamcervante added a commit that referenced this pull request Jul 2, 2024
@burnerlee
Copy link
Contributor Author

Whoops, I missed this was being merged directly into the v1.8 branch. We should have merged this into main and then let the backport bot automatically push into the most recent release branch. I should have checked the target branch more carefully.

I've reverted this PR in #35410, and merged this into the correct branch in #35411. @burnerlee, the commit that lands in main should still have your Github profile associated with it 👍

oh I see, I will take care of this in the future contributions

@burnerlee
Copy link
Contributor Author

@liamcervante, I am looking forward to making new contributions to this project. Can't find recent good-first-issues. Can you assign me some beginner-friendly/medium level issues?

@crw
Copy link
Collaborator

crw commented Jul 2, 2024

Hi @burnerlee, thanks for your contribution! We appreciate your enthusiasm, unfortunately we do not currently have a list of good first issues for community contribution. Please keep checking in for any issue marked good first issue for opportunities to contribute. Also please check out https:/hashicorp/terraform/blob/main/.github/CONTRIBUTING.md, if there are updates to the Contributing process I will update that document. Thanks very much!

@burnerlee
Copy link
Contributor Author

burnerlee commented Jul 2, 2024

Hi @burnerlee, thanks for your contribution! We appreciate your enthusiasm, unfortunately we do not currently have a list of good first issues for community contribution. Please keep checking in for any issue marked good first issue for opportunities to contribute. Also please check out https:/hashicorp/terraform/blob/main/.github/CONTRIBUTING.md, if there are updates to the Contributing process I will update that document. Thanks very much!

Hey @crw thanks for the reply. Can you assign me some medium level issues instead? I would be happy to go through the codebase and try to resolve those. I'm looking to be a long time contributor

@crw
Copy link
Collaborator

crw commented Jul 2, 2024

@burnerlee Unfortunately, we do not have the capacity on the team right now for managing and reviewing larger external contributions. In general, it is difficult to ramp up external contributors on the context needed to understand the impact of changing any particular system. Again, please see specifically https:/hashicorp/terraform/blob/main/.github/CONTRIBUTING.md#contributing-a-pull-request for more info on how to contribute effectively. I'd strongly recommend looking at the AWS or Azure providers as a starting place to make more widespread contributions.

Copy link

github-actions bot commented Aug 3, 2024

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 Aug 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.9-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants