CLOUDSTACK-9067 - As I developer I want to remove all the unused router-shell scripts from ACS #1084
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Java constants also removed - Project still compiling and all unit tests passing.
|
Ping @remibergsma @miguelaferreira @karuturi @DaanHoogland @borisroman Partial tests results, but I will still run some tests against XenServer 6.2 Cheers, :: Environment 1 ::
:: Tests Suites Executed :: :: Environment 2 ::
:: Tests Suites Executed :: :: Summary ::
(*) VM migration, I had only 1 host. :: Test results for Environment 1 :: :: Test results for Environment 2 :: |
| @@ -310,6 +310,11 @@ public static String mapToString(final Map<String, String> map) { | |||
| return listWPagination; | |||
| } | |||
|
|
|||
| public static String format(final String format, final Object... arguments) { | |||
There was a problem hiding this comment.
this is usefull for a lot of logging ;)
There was a problem hiding this comment.
Doesn't String.format(...) do the same thing?
Sorry, shouldn't have made that a question...
Why is it better to do StringUtils.format(...) instead of String.format(...)?
|
less is more! reviewed java code only, didn't check the removed scripts. LGTM |
|
@miguelaferreira discussed with me the over-engineering wrapper method just to add 1 test and we agreed on getting rid of it for the following reasons:
I will retest the PR and post the results here. Cheers, |
|
good points, I do want to have a more generic way of creating complex log messages though. might be useful to construct something like this for it anyway. (corrected my wpeech impediment) |
|
Ping @DaanHoogland @remibergsma @miguelaferreira @bhaisaab @karuturi @borisroman First round of tests done and got the same results! I will run some XenServer tests as well, but if you can help testing that will be much appreciated.
|
|
@wilderrodrigues maybe we can ask @anshul1886 to run the test wuite on hyperv? (my wpeech impdiment again, or is it wyping?) |
|
If @anshul1886 can test it, just to make sure all is fine, would be very nice! @karuturi, do you have a HyperV test environment? :) Cheers, |
|
More test results:
|
|
@wilderrodrigues nope. No HyperV environment. |
|
Thanks, @karuturi! I'm testing against XenServer 6.2 now. Given the changes on the HyperV Resource class, I'm not expecting problem there. The changes a very clear. Is there anyone willing to test this PR or the +1 from @DaanHoogland and all the tests I already executed will be enough to get a second +1? What do you think, @remibergsma ? Cheers, |
|
Ping @DaanHoogland @miguelaferreira @karuturi @borisroman @bhaisaab @remibergsma More test results, guys! It looks good to me... but my vote doesn't count. Still need 1 LGTM.
|
|
@wilderrodrigues LGTM 👍 Always in support of removing old/dead code! :) Testing: CentOS 7.1 Setup Managment/Hypervisor |
|
VR scripts got stabilised over years. If any thing wrong in new implementation there is chance to look into the VR scripts and update/correct the python methods. If the scripts are there in code it is easy for the developer to compare. So I feel keeping the scripts in code base is better.
|
|
@jayapalu That's where git's versioning system comes into play. We want to keep the current working directory as clean as possible. Meaning, without unused/dead code. If you want to compare how it was, use the history that git provides. |
|
@jayapalu, I'm with @borisroman on that one. We are no longer using shared file systems to keep files stored. We have a good version control system and the commit message is clear about what was done. We also have a very clear message in the Jira ticket, which was used in every commit message. The current Master/4.6 is stable and have been tested. if we face problems, we check the history. Cheers, |
|
I also agree we shouldn't keep unused files in the active tree. We already got some PRs against files that were not in use and that is a waste of time for everybody. Will merge soon. |
asfgit
pushed a commit
that referenced
this pull request
Nov 19, 2015
…UDSTACK-9067 CLOUDSTACK-9067 - As I developer I want to remove all the unused router-shell scripts from ACSThis PR removes the unused shell scripts that were present in the ACS project. Those script were replaced by the. Some of the scripts are used by the HyperV Resource, which were hardcoded. I took the opportunity to use the Java constants over there as well, so the next one touching the code will know they exist and won't hardcode anything. The following task were applied: * Remove the shell files and the Java constants that were mapping them; * Apply the use of the Java constants to the HyperV Resource class; * Wrap the String.format() method in the StringUtils so we can test the changes in the HyperV Resource class. The last point was added because I do not have a HyperV test environment. Hence, I wanted to make sure the tiny code I changed is covered at least by unit tests. * pr/1084: CLOUDSTACK-9067 - Replaces hardcoded paths with the VRScripts constants. CLOUDSTACK-9067 - Fomatting the code of HypervDirectConnectResource class CLOUDSTACK-9067 - Remove old script file from the project Signed-off-by: Remi Bergsma <github@remi.nl>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the unused shell scripts that were present in the ACS project. Those script were replaced by the.
Some of the scripts are used by the HyperV Resource, which were hardcoded. I took the opportunity to use the Java constants over there as well, so the next one touching the code will know they exist and won't hardcode anything.
The following task were applied:
The last point was added because I do not have a HyperV test environment. Hence, I wanted to make sure the tiny code I changed is covered at least by unit tests.