-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Rename CreateConditionCmd class to CreateConditionForVmAutoScalingCmd #11819
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
base: main
Are you sure you want to change the base?
Rename CreateConditionCmd class to CreateConditionForVmAutoScalingCmd #11819
Conversation
@erikbocks , sensible change but I would leave out the “Vm” bit from the new name. “CreateConditionForAutoScalingCmd” seems more appropriate as the VM is not scaled itself but the service is scaled by adding more (or less) VMs.What do you think? cc @weizhouapache @shwstppr . |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11819 +/- ##
============================================
+ Coverage 17.38% 17.56% +0.17%
- Complexity 15282 15498 +216
============================================
Files 5891 5898 +7
Lines 526356 527778 +1422
Branches 64270 64473 +203
============================================
+ Hits 91526 92702 +1176
- Misses 424488 424652 +164
- Partials 10342 10424 +82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
maybe use createAutoScalePolicy |
@DaanHoogland I don't have a strong preference either way. I don't even mind class remain CreateCondition as personally I find it easier during troubleshooting to search the class with API name. |
I like @weizhouapache ’s name best, |
I agree with the |
can you please also change the other classes for AS condition ? Update, Delete, List ? |
Description
Currently, the
createCondition
command belongs to a class named asCreateConditionCmd
. This class has a very abstract name and does not define what is the purpose of the condition. Thus, this PR renamesCreateConditionCmd
class toCreateConditionForVmAutoScalingCmd
, helping developers to understand the class purpose without having to access it.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
I built the packages with the changes, applied them in my local environment and called the API
createCondition
via CloudMonkey and validated that everything works as intended. Unitary tests were also executed and they passed successfully.How did you try to break this feature and the system with this change?