-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
enable_partitioning not supported servicebus Premium Sku. #1391
Conversation
// We need to retrieve the namespace because Premium namespace works differently from Basic and Standard, | ||
// so it needs different rules applied to it. | ||
namespacesClient := meta.(*ArmClient).serviceBusNamespacesClient | ||
namespace, nsErr := namespacesClient.Get(ctx, resourceGroup, namespaceName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor, but can we make nsErr just err to keep it consistent in the codebase?
return fmt.Errorf("ServiceBus Queue (%s) must have Partitioning enabled for Premium SKU", name) | ||
// In a Premium tier namespace, partitioning entities is not supported. | ||
if namespace.Sku.Name == servicebus.Premium && d.Get("enable_partitioning").(bool) { | ||
return fmt.Errorf("ServiceBus Queue (%s) does not support Partitioning enabled for Premium SKU", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are changing the behavior can we update the documentation too with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gonna push a commit to remove this, since existing Namespaces should continue working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just needs a few minor tweaks and I think it will be good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lrxtom2,
Thank you for the PR, however I think we need to proceed carefully here. As per the documentation you linked:
Partitioning is available at entity creation for all queues and topics in Basic or Standard SKUs. It is not available for the Premium messaging SKU, but any previously existing partitioned entities in Premium namespaces continue to work as expected.
So it sounds like it used to be only available for premium, and now its available for all tiers except premium. If we do the change as you suggest it could un-partition existing entities. I'm not sure of the best way forward, but we need to make sure we don't force a change upon users who don't want it yet.
So we need to make sure that enable_partitioning
can be enabled for premium on import/update/read while only enforcing the condition on create.
In addition we will then need to check on create to make sure we don't upsert over an existing resource (this is something that we need to do anyways, but in this case its extra important)
@katbyte FWIW we'd tend to normally leave this as a supported property and add a |
@@ -165,7 +165,7 @@ func TestAccAzureRMServiceBusTopic_enablePartitioningPremium(t *testing.T) { | |||
{ | |||
Config: postConfig, | |||
Check: resource.ComposeTestCheckFunc( | |||
resource.TestCheckResourceAttr(resourceName, "enable_partitioning", "true"), | |||
resource.TestCheckResourceAttr(resourceName, "enable_partitioning", "false"), |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't possible due to the API behaviour change/limitation
@lrxtom2 what's user impact if we don't make the fix? Is enable_partitioning still valid for Premium at API level? |
No longer failing if the sku is premium to allow for existing resources
// In a Premium tier namespace, partitioning entities is not supported. | ||
if namespace.Sku.Name == servicebus.Premium && d.Get("enable_partitioning").(bool) { | ||
return fmt.Errorf("ServiceBus Queue (%s) does not support Partitioning enabled for Premium SKU", name) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gonna push a commit to remove this, since existing Namespaces should continue working
changes have been pushed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
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! |
According to azure-doc last update, enable_partitioning is not supported in a Premium tier namespace.
reference:
In a Premium tier namespace, partitioning entities is not supported. However, you can still create Service Bus queues and topics in 1, 2, 3, 4, 5, 10, 20, 40, or 80-GB sizes (the default is 1 GB). You can see the size of your queue or topic by looking at its entry on the Azure portal, in the Overview blade for that entity.
https://docs.microsoft.com/en-us/azure/service-bus-messaging/service-bus-partitioning