Service endpoint alias fix #57
Conversation
There was a problem hiding this comment.
Kudos for tackling this @lakkeger ! 🎉 (Also discovered this recently, and had to put in a local quick fix to prepare for a demo.. ;) )
Added a few suggestions and clarification questions.. Let me know if anything is unclear, happy to discuss more.
| print(f'Unable to parse "{override_file}" as HCL file: {e}') | ||
|
|
||
| endpoints = result["endpoints"][0] | ||
| if "config" in endpoints and "configservice" in endpoints: |
There was a problem hiding this comment.
Instead of hardcoding the service names here, could we pass in a list of expected services as a parameter of the function?
Also, I would have expected this to be something like this - wouldn't we expect only one of the two be contained in the endpoints list (and not both)..? 🤔
if "config" not in endpoints and "configservice" in endpoints:
There was a problem hiding this comment.
Sure, not a problem to use them es parameters.
However I'd argue the logic change here:
If the list order changes in tflocal that breaks the test for no reason, and not necessarily wrong to use configservice instead of config or vice versa as it's a valid option, we just want to use exclusively one in any given point to hide the warnings, so we made the decision to do it with the first item from the aliases in tflocal.
Or do you think we should focus more here to test the actual values we should see in the generated file?
There was a problem hiding this comment.
Ok. I think the logic makes sense, thanks for clarifying. Maybe we could still make the logic parametrizable, rather than harcoding the service names.
Btw, if I understand correctly, we want to ensure that no duplicates are being generated for the service endpoints - could we simply iterate over all alias pairs within SERVICE_ALIASES, and ensure that only one of the aliases is defined in the config..? 🤔
There was a problem hiding this comment.
That's already happenning here :)
Line 150 in 6d9f75b
Basically we iterate through all aliases, if the alias is among the service endpoints we add the service endpoint by the first alias name so in case we find it multiple times we simply just updating the service endpoint dict at the respective key.
There was a problem hiding this comment.
Yep, that makes sense, great to see that we have this logic in place for create_provider_config_file. 👍 I was more referring to the test assertion here, if we are deliberately checking for config/configservice only. But if we're happy with the assertion here, I'm fine with merging as-is (we can evolve this as we go.. ➿ ).
|
Found a bug with the new v5.55.0 aws provider, hence the timeout in the pipelines. To apply a temporary fix I changed Related exception in LocalStack: And the debug log line in Terraform where it happens: |
Motivation
Issue #55 pointed out the warning below:
The source of it is the generated provider file contains endpoints which are each other's aliases.
Changes