application_insights_api_key - Fixes infinite loop while checking for existing api keys

[TwinbrotherPro]

Community Note

• Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
• Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

If there are already existing API Keys present in the given appinsights resource which should not be managed by Terraform the code does no break in the given for loop as we never run into one of the two error cases.

I think the reason for this infinite outer for loop is that it actually is a typo and is meant to be an if statement. Which would fix the bug and make much more sense in my eyes.

PR Checklist

• I have followed the guidelines in our Contributing Documentation.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
• I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
• I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
  For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

• I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
• I have written new tests for my resource or datasource changes & updated any relevent documentation.
• I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
• (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

• My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

• application_insights_api_key - fix infinite loop while checking for existing api keys

This is a (please select all that apply):

• Bug Fix
• New Feature (ie adding a service, resource, or data source)
• Enhancement
• Breaking Change

Related Issue(s)

• No issue created.

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

[TwinbrotherPro]

Open questions from my side:

• I might find the time to write a proper test for the found bug
• Do I need to create an issue or is the PR enough?

[wyattfry]

Would it make sense to add an else, now that it's an if, with some logging info or handling?

[wyattfry]

I also wonder if the original for was just polling until the condition is true, if so, might be nice to leave the for but if it reaches the max attempts then errors out

[TwinbrotherPro]

I also wonder if the original for was just polling until the condition is true, if so, might be nice to leave the for but if it reaches the max attempts then errors out

Thanks for your reply. I am not 100% familiar with the appinisghts go and the azure client go sdk. Can you quickly explain why you think the APIKeysList method could be asynchronous?

If there is an api-key which is not under terraform state control and also not defined as a future resource this for loop will poll indefinitely.
I could add a condition for checking for a max attempt threshold but would only agree to it if APIKeysList is truly async.

[stephybun]

Looks like I introduced this bug when swapping the appinsights service over to using our SDK, sorry 😓! This should definitely not be looping on those conditions.

Thanks for fixing this @TwinbrotherPro! LGTM ⚡

[TwinbrotherPro]

Thanks for taking over @stephybun. Looking forward to v4.12.0 👍