[Network]Fix 10854 associate rule set to request routing rules #11421
Conversation
src/azure-cli/HISTORY.rst
Outdated
There was a problem hiding this comment.
Do we need add completer=get_ag_subresource_completion_list() for "rewrite_rule_set"?
There was a problem hiding this comment.
what's practice to set min_api version? I thought it's for azure stack. is it possible to specify version at global level, instead of in business logic code? it's hard for maintenance.
There was a problem hiding this comment.
Based on my current knowledge, the current practice is to set min_api version like these across services like network/storage. We can discuss a solution to set the version at global level.
There was a problem hiding this comment.
again, it looks really bad for adding hardcode versions in different piece of codes but same purpose. at least define something like global var rewrite_rule_set_min_version
There was a problem hiding this comment.
A possible reason behind hardcode might be if we need to define a global var, the name might be ag_url_path_map_rewrite_rule_set_min_version. This api-version is limited in these two commands az network application-gateway url-path-map create/update
There was a problem hiding this comment.
@yungezz Currently, we set min_api=2019-04-01 in _param.py too. I just checked the structure in cmd and we can also get this information. How about we support something like cmd.support_parameter(rewrite_rule_set) so that we can remove this hardcode version and keep the version information limited in _param.py
There was a problem hiding this comment.
It can also work for multi api if I write the check like if rewrite_rule_set is None
There was a problem hiding this comment.
thanks @myronfanqiu . sounds good to add in cmd. Image one day, you'll need update versoin for a feature like rewrite_rule_set, how it's possible to find out all instance of hardcode version. meanwhile, bad readability.
Eventually, I would like we have this practice tested/verified, and make it into cli command guideline.
|
@yungezz Hello. I included two kinds of solution in this PR. After re-thinking the structure, I think |
There was a problem hiding this comment.
just a wild thinking, do you think it make sense that cli might be asked to have multiple min_api_version, such as one for azure stack, one for JEDI, one for moon_cake?
There was a problem hiding this comment.
Currently, min_api is based on the first time this param appears in python SDK/swagger spec. Unless we need to choose different package version(not api-version) of sdk for different cloud, I cannot image we need to support multiple min_api_version.
There was a problem hiding this comment.
Since min API version is constant (will not change in the future), I prefer hard-code it to eliminate the over-complexity.
There was a problem hiding this comment.
it's not about one api version, pls look at the whole file, or cli codes in general, lots of api version, even in one rp, one resource, min_api_version is on property level. This introduces bad readability and heavy maintenance effort. purpose of the comments is general improving code readability, maintenance. while for how to generalize the way, we can discuss. that's the context why @myronfanqiu provided 2 options open for discussion. If more concern, let's discuss offline.
mmyyrroonn
force-pushed
the
fix-10854-associate-rule-set-to-request-routing-rules
branch
from
83b08b2 to
3ab8040
Compare
mmyyrroonn
force-pushed
the
fix-10854-associate-rule-set-to-request-routing-rules
branch
from
3ab8040 to
71b58a2
Compare
mmyyrroonn
deleted the
fix-10854-associate-rule-set-to-request-routing-rules
branch
5 participants
Fix #10854 #10869
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.