[EngSys] updating the delete methods - #18304
Conversation
|
|
||
| def remove_resource(self, name, **kwargs): | ||
| if self.is_live and self._need_creation: | ||
| name = u"rgpy-" + name |
There was a problem hiding this comment.
Oh this must have broken ever since you added the prefix. I was under an incorrect assumption when I responded to issue I thought they were hardcoding something in the test code.
It might be worth putting this name logic in a common function that can be shared between create and delete.
There was a problem hiding this comment.
I don't think adding this hardcode tricky logic in both create_resource and remove_resource is a good way. Maybe you can consider adding prefix for the name before you pass name parameter into create_resource and remove_resource.
There was a problem hiding this comment.
Also the log shows the wrong name (the name without prefix), actually we create resource with prefix.
logging.info(
"Attempting to create a Resource Group with name {} and parameters {}".format(name, parameters)
)
# Prefixing all RGs created here with 'rgpy-' for tracing purposes
name = u"rgpy-" + name
if len(name) > 90:
name = name[:90]
self.resource = self.client.resource_groups.create_or_update(name, parameters)There was a problem hiding this comment.
I don't think adding this hardcode tricky logic in both
create_resourceandremove_resourceis a good way. Maybe you can consider adding prefix for the name before you pass name parameter intocreate_resourceandremove_resource.
Yu Chen (@jsntcy) I don't understand what you mean by this, can you elaborate or provide a code snippet?
There was a problem hiding this comment.
I'm not sure we would want to update all the callers of create/remove resources to have them pass in the name with the prefix but I do think it is a good idea to factor the name updates into a common function that we call at the beginning of both create and remove resources.
Also from a quick look it seems as though there is already a name_prefix value in this class which serves a slight different but similar purpose. We might want to reconcile the concepts and perhaps have a rgname_prefix or something along those lines.
There was a problem hiding this comment.
Yes, agree with Wes Haggard (@weshaggard), since both create_resource and remove_resource are in class ResourceGroupPreparer, you can define a variable in class which can be shared among all functions in class ResourceGroupPreparer.
Not sure if we can reuse self.name_prefix which already exists in class ResourceGroupPreparer.
https://github.com/Azure/azure-sdk-for-python/blob/master/tools/azure-sdk-tools/devtools_testutils/resource_testcase.py#L62
BTW, could you tell more about "add this prefix for tracking purpose"? Sean Kane (@seankane-msft)
There was a problem hiding this comment.
Yu Chen (@jsntcy) a few weeks ago our Subscription ID hit the maximum number of resource groups allowed, and we had no information on hundreds of RGs. Wes and I added the prefix so we could easily determine where at least some of the generated RGs were coming from.
Yu Chen (jsntcy)
left a comment
Yu Chen (jsntcy)
left a comment
There was a problem hiding this comment.
Please take a look at the comments.
|
Hello Sean Kane (@seankane-msft)! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
/check-enforcer override |
“â[Go]Add batch tag 22/03/17 (Azure#18304)
“â[Go]Add batch tag 22/03/17 (Azure#18304)
“â[Go]Add batch tag 22/03/17 (Azure#18304)
4 participants
#18287