feat(Azure): #1491 azure container apps support#1493
feat(Azure): #1491 azure container apps support#1493james00012 merged 4 commits intogruntwork-io:mainfrom
Conversation
Add support for asserting and getting azure container apps. Added tests and examples
6b4dd58 to
a8366d7
Compare
james00012
left a comment
There was a problem hiding this comment.
Will also trigger test pipeline to see if it all passes.
|
|
||
| func getClientCloudConfig() (cloud.Configuration, error) { | ||
| envName := getDefaultEnvironmentName() | ||
| switch strings.ToUpper(envName) { |
There was a problem hiding this comment.
What if the envName doesn't match any known environment, it returns an error. You could consider logging the defaulting behavior or providing guidance on what values are expected.
There was a problem hiding this comment.
I could find any good examples on how logging is done without having the testing.T at hand with terratest.modules.logger. Do you have any preferences or is it sufficient to just add enough context to the error returned?
There was a problem hiding this comment.
Hmm can you just pass in the testing.T variable along and share the context that weay?
There was a problem hiding this comment.
That would require all the funtions XxxxE like ManagedEnvironmentExistsE to break the pattern from all other azure test functions and have t testing.T as an argument. My initial thought behind just erroring out is that the value defaults to AzurePublicCloud and if the user has set the env var to a invalid value the test should fail fast and return an error with the reason. This behavior also seems to follow the same pattern as the getEnvironmentEndpointE function. If my concerns around the different function pattern and fail fast with error is wrong I'm more than happy to make those changes
There was a problem hiding this comment.
That's a valid point. We could look into better logging pattern in a separate issue.
* remove unknown comments * handle error from getTargetAzureSubscription * improve error text if cloud env value not known (until preferred logging solution)
|
Friendly ping on the comment above @tjololo |
|
You would have to click the "update branch" before merging |
Description
Add support for asserting and getting azure container apps. Added tests and examples
Fixes #1491
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added support for azure container apps related get and assertion
Migration Guide