Add servicebus #525
Add servicebus #525
Conversation
|
Thanks for your contribution! Can you also add some tests for the functions, either unit or integration? Note that while we want to add more support for Azure in terratest, we need some help to maintain the code, and we aren't planning on merging Azure contributions until we can figure out a way to run builds on Azure. Please take a look at #89 (comment) to learn more. |
|
Please see #89 for the status on Terratest with Azure. In particular, we're starting some work around it, and when that work is done, we'll be able to come back to this PR! |
Add tests and terraform module for service bus
|
@HadwaAbdelhalem The PR is ready for review |
|
Issue #771 |
|
New Run from CI/CD --> https://github.com/helayoty/terratest/runs/1786762894?check_suite_focus=true |
|
@HadwaAbdelhalem the latest CI/CD result --> https://github.com/helayoty/terratest/runs/1835742059?check_suite_focus=true |
| } | ||
| } | ||
| } | ||
| description = "All topics with the corresponding subscriptions" |
There was a problem hiding this comment.
NIT: description should be first in the output block to be consistent with the other outputs.
modules/azure/servicebus.go
Outdated
| return &sClient, nil | ||
| } | ||
|
|
||
| // ListServiceBusNamespaceE list all SB namespaces in all resource groups in the given subscription ID. This function would fail the test if there is an error. |
There was a problem hiding this comment.
The comment is incorrect with the function (E flavor returns errors, not fail the test). This applies to all functions in this file.
| // ListServiceBusNamespaceE list all SB namespaces in all resource groups in the given subscription ID. This function would fail the test if there is an error. | |
| // ListServiceBusNamespaceE list all SB namespaces in all resource groups in the given subscription ID. |
modules/azure/servicebus.go
Outdated
| return &results, nil | ||
| } | ||
|
|
||
| // ListServiceBusNamespace - list all SB namespaces in all resource groups in the given subscription ID. |
There was a problem hiding this comment.
NIT: Non-E flavored functions should include the text that this would fail the test. This applies to all functions in this file.
modules/azure/servicebus.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| results := make([]string, 0) |
There was a problem hiding this comment.
NIT: If we're using make, then this should use len(*sbNamespace) as the length/capacity and then set the values directly. Otherwise, it's more idiomatic to use results := []string{} or var results []string to define an empty slice.
modules/azure/servicebus.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
NIT: this code block is repeated between ListServiceBusNamespaceNamesE and ListServiceBusNamespaceNamesByResourceGroupE. Can you create an internal helper that plucks the namespace names given a slice of namespaces?
This comment applies also to ID attribute flavor.
modules/azure/servicebus.go
Outdated
| } | ||
|
|
||
| // ListServiceBusNamespaceE list all SB namespaces in all resource groups in the given subscription ID. This function would fail the test if there is an error. | ||
| func ListServiceBusNamespaceE(subscriptionID string) (*[]servicebus.SBNamespace, error) { |
There was a problem hiding this comment.
Can you switch these slice pointer return values to simple slices (applies to all functions in this file)? Note that slices are already pointers so you can still return nil.
In general, we try to avoid usage of pointers, especially pointers to slices as it leads to increased complexity unless it is necessary. For example, in this case you will have to do nil checks whenever you access these slices as nil is no longer equivalent to [] and the dereference in the range calls can lead to panics. Additionally, slice pointers are most useful when you want to pass by reference, and the use case for that is usually limited to function args.
|
|
||
| results := make([]string, 0) | ||
| for _, namespace := range *sbNamespace { | ||
| results = append(results, *namespace.Name) |
There was a problem hiding this comment.
Use safePtrToString when dereferencing string ptr attributes.
There was a problem hiding this comment.
@HadwaAbdelhalem looks like this comment wasn't addressed. I'm assuming that's because there is some guarantee in the SDK that ensures this is set. Is that correct assumption?
There was a problem hiding this comment.
@yorinasub17 it is, the SDK is calling the Azure Rest API underneath for all resources operations, and the validation of the name as a required parameter is done on the rest API Here
modules/azure/servicebus.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This should be removed as there is no new err definition in the for loop.
modules/azure/servicebus.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This should be removed as there is no new err definition in the for loop.
modules/azure/servicebus.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This should be removed as there is no new err definition in the for loop.
modules/azure/servicebus.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This should be removed as there is no new err definition in the for loop.
modules/azure/servicebus.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This should be removed as there is no new err definition in the for loop.
|
Hi @yorinasub17 I updated the PR to address your comments, please review when you get a chace. Here is a link to the CI . the failure is related to the ACI test and I will fix in a separate PR. |
|
|
||
| results := make([]string, 0) | ||
| for _, namespace := range *sbNamespace { | ||
| results = append(results, *namespace.Name) |
There was a problem hiding this comment.
@HadwaAbdelhalem looks like this comment wasn't addressed. I'm assuming that's because there is some guarantee in the SDK that ensures this is set. Is that correct assumption?
modules/azure/servicebus.go
Outdated
|
|
||
| for iteratorSubscription.NotDone() { | ||
| results = append(results, iteratorSubscription.Value()) | ||
| err = iteratorSubscription.Next() |
There was a problem hiding this comment.
NIT: This should really be err := iteratorSubscription.Next(), or even better is:
if err := iteratorSubscription.Next(); err != nil {
return nil, err
}
so that it defines a new err var scoped to the forloop/if block. This is a general go pattern to avoid mistakes arising from overriding variables unintentionally.
This applies to the other sections as well.
|
Ah looks like the precommit hook failed on running Here is the diff: diff --git a/modules/azure/servicebus_test.go b/modules/azure/servicebus_test.go
index 25c3aad4..f96077dd 100644
--- a/modules/azure/servicebus_test.go
+++ b/modules/azure/servicebus_test.go
@@ -18,7 +18,7 @@ func TestListServiceBusNamespaceNamesE(t *testing.T) {
subscriptionID := ""
- _, err := ListServiceBusNamespaceNamesE(subscriptionID)
+ _, err := ListServiceBusNamespaceNamesE(subscriptionID)
require.Error(t, err)
}
@@ -28,7 +28,7 @@ func TestListServiceBusNamespaceIDsByResourceGroupE(t *testing.T) {
subscriptionID := ""
resourceGroup := ""
- _, err := ListServiceBusNamespaceIDsByResourceGroupE(subscriptionID, resourceGroup)
+ _, err := ListServiceBusNamespaceIDsByResourceGroupE(subscriptionID, resourceGroup)
require.Error(t, err)
}
@@ -39,7 +39,7 @@ func TestListNamespaceAuthRulesE(t *testing.T) {
namespace := ""
resourceGroup := ""
- _, err := ListNamespaceAuthRulesE(subscriptionID, namespace, resourceGroup)
+ _, err := ListNamespaceAuthRulesE(subscriptionID, namespace, resourceGroup)
require.Error(t, err)
}
@@ -50,7 +50,7 @@ func TestListNamespaceTopicsE(t *testing.T) {
namespace := ""
resourceGroup := ""
- _, err := ListNamespaceTopicsE(subscriptionID, namespace, resourceGroup)
+ _, err := ListNamespaceTopicsE(subscriptionID, namespace, resourceGroup)
require.Error(t, err)
}
@@ -62,7 +62,7 @@ func TestListTopicAuthRulesE(t *testing.T) {
resourceGroup := ""
topicName := ""
- _, err := ListTopicAuthRulesE(subscriptionID, namespace, resourceGroup, topicName)
+ _, err := ListTopicAuthRulesE(subscriptionID, namespace, resourceGroup, topicName)
require.Error(t, err)
}
@@ -76,4 +76,4 @@ func TestListTopicSubscriptionsNameE(t *testing.T) {
_, err := ListTopicSubscriptionsNameE(subscriptionID, namespace, resourceGroup, topicName)
require.Error(t, err)
-}
\ No newline at end of file
+} |
There was a problem hiding this comment.
Passed the precommit hooks! @HadwaAbdelhalem Let me know if this is ready to merge (in particular, if the Azure build passed for you).
|
Thank you @yorinasub17 for checking it. it is ready for merge, the CI has a failed test that is unrelated to this PR. It is the TestTerraformAzureACIExample is failing duo to a change in the MS container images move to MCR . I created an Issue to track the ACI failure and will submit a fix for later today |
|
Thanks! Will merge this in and release! |
Add Service Bus Azure module to terratest