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

Nil refence fix for issue #464 #492

Merged
merged 1 commit into from
Nov 3, 2017
Merged

Nil refence fix for issue #464 #492

merged 1 commit into from
Nov 3, 2017

Conversation

madenwala
Copy link
Contributor

This nil check is needed to prevent a terraform crash that occurs when a user tries to update a network rule that was manually changed through the Azure Portal. Fix is to check if properties on the props object is nil before you try to assign those string values to the sgRule dictionary.

@madenwala madenwala merged commit 1686648 into hashicorp:master Nov 3, 2017
@thiagocaiubi
Copy link
Contributor

thiagocaiubi commented Nov 3, 2017

Hi @madenwala . Thanks for the work!

Why this PR was self-approved and why it lacks tests?

@madenwala
Copy link
Contributor Author

Sorry about that, I'm new to this. I didn't know that I shouldn't submit the merge. Not exactly sure how to create a test case around this as the crash only occurs if you manually edit the fields through the Azure portal and then try to run your TF script again. The fix is very simple, just adding a nil check before you try to ready the property value.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Taking a look at this retroactively, this change LGTM - however as discussed on Slack this shouldn't have been merged without a review and an acceptance test to cover it. I've enabled Branch Protection for the moment which requires a 👍 before merging - which should mean this can't happen in the future.

In addition - as discussed it'd be good to add an acceptance test for this in a follow-up PR - if you can reach out we can talk through the best way to achieve this :)

Thanks!

@tombuildsstuff
Copy link
Contributor

Hey @thiagocaiubi

Why this PR was self-approved and why it lacks tests?

Thanks for raising this - in addition to @madenwala's comments I've followed up internally and have enabled Branch Protection on the master branch (which requires a 👍 from an admin) which should avoid this issue going forward.

Regarding the missing tests - I've reached out to @madenwala and we're going to discuss the options here and add these in a follow up PR.

Thanks!

@thiagocaiubi
Copy link
Contributor

thiagocaiubi commented Nov 7, 2017

Awesome @tombuildsstuff! Great work! 👏

Thanks!

@ghost
Copy link

ghost commented Apr 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants