-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
CLOUDSTACK-9315: Removed unused Classes #1448
CLOUDSTACK-9315: Removed unused Classes #1448
Conversation
@GabrielBrascher removing unused code is good; less is more. These are API calls and related stuff however. A large test set should be run and I am surprised no test code would be touched by this change. |
@DaanHoogland the lack of test in some classes indeed is a problem. However, these classes that I removed are not being used. I used UCDetector (https://www.ucdetector.org/) as a plugin for Eclipse. With this tool, I discovered a lot of code without any reference (variables, methods and classes). This pull request had the goal of removing some of these classes. To ensure that I wasn't missing anything I searched for any file that could reference some answer or API command. As I haven't found any way of these classes being used, I removed them. Any of those API commands that I deleted are not using org.apache.cloudstack.api.APICommand (with |
Ok, that is somewhat comforting, however still a large integration test suite should be run on this. |
With these major stripping out of unused code, we need to be very careful that we do not introduce integration test issues. Can you please validate that the integration test output does not change based on this code being removed? |
@DaanHoogland @swill thank you for the concern. I agree with the need for executing integration/functional tests to keep the code stable. Unfortunately, our test environment is not set yet and it may take a while. As soon as we have it operational I will run the test. |
Thank you @GabrielBrascher. I am also getting my CI setup so I can get tests running against these PRs, so I should be able to start validating this stuff as well. Thanks for the continued effort. :) |
…asses2 Remove classes with no referencesI used UCDetector (https://www.ucdetector.org/) as a plugin for Eclipse. With this tool, I discovered a lot of code without any reference (variables, methods and classes). Following the work that was done at [#1448]; this pull request had the goal of removing some of these classes. To check if I wasn't missing anything I searched for any file that could reference some of those classes. As I haven't found any way of these classes being used, they were removed. Note that some of them I found other references, but references such as commented lines or tests, nothing that could indicate their use (as XML files configuring beans or another class instantiating an object with "new"). Waiting for tests. Please tell me if I am missing something. Removed Classes: - org.apache.cloudstack.framework.jobs.JobCancellationException (**Note:** removed variable JobCancellationException in com.cloud.utils.SerialVersionUID) - org.apache.cloudstack.ldap.NoSuchLdapUserException (**Note:** removed test file /cloud-plugin-user-authenticator-ldap/test/groovy/org/apache/cloudstack/ldap/NoSuchLdapUserExceptionSpec.groovy) - com.cloud.agent.api.storage.CreateVolumeOVAAnswer - com.cloud.exception.MissingParameterValueException - org.apache.cloudstack.api.response.StatusResponse - org.apache.cloudstack.api.response.VolumeDetailResponse - org.apache.cloudstack.api.response.UpgradeVmResponse - org.apache.cloudstack.api.response.AddIpToVmNicResponse - org.apache.cloudstack.api.response.TemplateZoneResponse (**Note:** at org.apache.cloudstack.api.response.TemplateResponse, there is this comment "To avoid breaking backwards compatibility, we still treat a template at different zones as different templates, so not embedding template_zone information in this TemplateZoneResponse set. `private Set<TemplateZoneResponse> zones;`" but right now it is not used) - org.apache.cloudstack.api.response.NicDetailResponse * pr/1453: Removed classes with no reference Signed-off-by: Will Stevens <[email protected]>
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests:
Skipped tests: Passed test suits: |
@GabrielBrascher can you rebase against latest master |
ce405b0
to
1df846e
Compare
Rebased against latest master. Thanks @rhtyd. |
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1063 |
@rhtyd can you please run integration tests? Thanks ;) |
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1112 |
"If set to true, DhcpEntryCommand, SavePasswordCommand, UserDataCommand, VmDataCommand will be synchronized on the agent side." | ||
+ " If set to false, these commands become asynchronous. Default value is false.", | ||
null), | ||
"If set to true, DhcpEntryCommand, SavePasswordCommand, VmDataCommand will be synchronized on the agent side." |
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.
@GabrielBrascher can you fix indentations changes in this class/file?
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.
Fixed, thanks @rhtyd.
@blueorangutan test |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
25e0c22
to
b0e0cba
Compare
b0e0cba
to
bfa9be6
Compare
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1413 |
@blueorangutan package @rhtyd I took the liberty of getting this PR and fixing the conflicts. There were quite a few unused classes that we removed. |
@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2351 |
@@ -1836,9 +1836,9 @@ | |||
Boolean.class, | |||
"execute.in.sequence.network.element.commands", | |||
"false", | |||
"If set to true, DhcpEntryCommand, SavePasswordCommand, UserDataCommand, VmDataCommand will be synchronized on the agent side." |
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.
Also found some reference on the schema-410to420.sql
file
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.
Good catch. I will fix that, and create a script to update the database.
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. Verified that classes are unused, just left some minor comment
@blueorangutan test |
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
…mmands' param Update description of 'execute.in.sequence.network.element.commands'parameter to reflect an unused command that has been removed. The removed class command is 'UserDataCommand'.
28093db
to
b65d820
Compare
@nvazquez do you know what happened with the tests? |
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2354 |
@rafaelweingartner was a failure downloading dependencies before environment has been built, let me start them again |
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-3085)
|
@rhtyd can I consider these tests results as successful? |
|
||
-- PR#1448 update description of 'execute.in.sequence.network.element.commands' parameter to reflect an unused command that has been removed. The removed class command is 'UserDataCommand'. | ||
update configuration set description = 'If set to true, DhcpEntryCommand, SavePasswordCommand, VmDataCommand will be synchronized on the agent side. If set to false, these commands become asynchronous. Default value is false.' where name = 'execute.in.sequence.network.element.commands'; |
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.
Can you change this to cloud configuration?
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.
I already did it (https://github.com/apache/cloudstack/pull/1448/files#diff-69537f9fb58dcc5d2b95bb427e8e47e5). I am only creating this script to update that values for environments that already have this entry in the database.
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.
@rafaelweingartner not the comment is to rewrite the update statement as:
UPDATE `cloud`.`configuration` SET ...;
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.
Ah, sure the schema. Done!
Yes @rafaelweingartner test lgtm, the failure are introduced by something else on master. I've left a minor note. |
Remove unused Classes:
com.cloud.configuration.Config.ExecuteInSequenceNetworkElementCommands
enum
//FIXME: Should have an DestroyAnswer
atcom.cloud.storage.resource.StoragePoolResource