Skip to content
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

cleanups as suggested by errorprone #7088

Merged
merged 7 commits into from Oct 4, 2023
Merged

Conversation

DaanHoogland
Copy link
Contributor

@DaanHoogland DaanHoogland commented Jan 12, 2023

Description

This PR applies errorprone as in below diff and applies the suggested changes for the first few projects. The permanent application of errorProne is removed within the scope of this PR as most projects will give positives.

commit 9f9a2b6b7e094537fea75f633d355f983d4c15d7
Author: Daan Hoogland <daan@onecht.net>
Date:   Thu Jan 12 19:32:35 2023 +0100

    errorprone

diff --git a/pom.xml b/pom.xml
index 204c74cc76..13c46d7521 100644
--- a/pom.xml
+++ b/pom.xml
@@ -817,6 +817,32 @@
                     </execution>
                 </executions>
             </plugin>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-compiler-plugin</artifactId>
+                <version>${cs.compiler-plugin.version}</version>
+                <configuration>
+                    <source>8</source>
+                    <target>8</target>
+                    <encoding>UTF-8</encoding>
+                    <compilerArgs>
+                        <arg>-XDcompilePolicy=simple</arg>
+                        <arg>-Xplugin:ErrorProne</arg>
+                    </compilerArgs>
+                    <annotationProcessorPaths>
+                        <path>
+                            <groupId>com.google.errorprone</groupId>
+                            <artifactId>error_prone_core</artifactId>
+                            <version>2.18.0</version>
+                        </path>
+                        <!-- Other annotation processors go here.
+
+                        If 'annotationProcessorPaths' is set, processors will no longer be
+                        discovered on the regular -classpath; see also 'Using Error Prone
+                        together with other annotation processors' below. -->
+                    </annotationProcessorPaths>
+                </configuration>
+            </plugin>
         </plugins>
         <pluginManagement>
             <plugins>
@@ -1074,15 +1100,26 @@
                     <configuration>
                         <source>${cs.jdk.version}</source>
                         <target>${cs.jdk.version}</target>
-                        <fork>true</fork>
-                        <meminitial>128m</meminitial>
-                        <maxmem>512m</maxmem>
+                        <encoding>UTF-8</encoding>
                         <compilerArgs>
                             <arg>-XDignore.symbol.file=true</arg>
                             <arg>--add-opens=java.base/java.lang=ALL-UNNAMED</arg>
                             <arg>--add-exports=java.base/sun.security.x509=ALL-UNNAMED</arg>
                             <arg>--add-exports=java.base/sun.security.provider=ALL-UNNAMED</arg>
+                            <arg>-XDcompilePolicy=simple</arg>
+                            <arg>-Xplugin:ErrorProne</arg>
+                            <arg>--illegal-access=warn</arg>
                         </compilerArgs>
+                        <annotationProcessorPaths>
+                            <path>
+                                <groupId>com.google.errorprone</groupId>
+                                <artifactId>error_prone_core</artifactId>
+                                <version>2.18.0</version>
+                            </path>
+                        </annotationProcessorPaths>
+                        <fork>true</fork>
+                        <meminitial>128m</meminitial>
+                        <maxmem>512m</maxmem>
                     </configuration>
                 </plugin>
                 <plugin>

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • CI/build

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Testing should be usual regression and monkey testing

@DaanHoogland
Copy link
Contributor Author

no time to go through all the projects now, I will stop working on this for a while. I think it is worth the trouble though. Any additions/suggestions are welcome.

@rohityadavcloud
Copy link
Member

@DaanHoogland if codecov, sonar are enough should we drop this? I had proposed this but recently I've found the project backers were let go and this may require more effort than initially thought.

@DaanHoogland
Copy link
Contributor Author

@DaanHoogland if codecov, sonar are enough should we drop this? I had proposed this but recently I've found the project backers were let go and this may require more effort than initially thought.

That is too bad, but if so, we should drop it or change the code to not require errorprone. I think most of the changes on code are legit, so we could revert just the pom and leave the rest so far in.

@rohityadavcloud
Copy link
Member

Thanks @DaanHoogland given we've sonar and codecov, we can revisit this in future; you may close the PR.

@DaanHoogland DaanHoogland changed the title errorprone cleanups as suggested by errorprone Jul 3, 2023
@DaanHoogland DaanHoogland marked this pull request as ready for review July 3, 2023 07:54
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #7088 (a3a24ea) into main (c6237c4) will increase coverage by 0.11%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #7088      +/-   ##
============================================
+ Coverage     13.37%   13.48%   +0.11%     
- Complexity     9321     9686     +365     
============================================
  Files          2740     2747       +7     
  Lines        258255   266113    +7858     
  Branches      40237    43640    +3403     
============================================
+ Hits          34542    35891    +1349     
- Misses       219353   225621    +6268     
- Partials       4360     4601     +241     

see 62 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@apache apache deleted a comment from blueorangutan Jul 18, 2023
@apache apache deleted a comment from blueorangutan Jul 18, 2023
@DaanHoogland
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6503

@DaanHoogland
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-7109)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 798 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7088-t7109-kvm-centos7.zip
Smoke tests completed. 0 look OK, 0 have errors, 113 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
all_test_safe_shutdown Skipped --- test_safe_shutdown.py
all_test_scale_vm Skipped --- test_scale_vm.py
all_test_2fa Skipped --- test_2fa.py
all_test_regions Skipped --- test_regions.py
all_test_accounts Skipped --- test_accounts.py
all_test_nonstrict_affinity_group Skipped --- test_nonstrict_affinity_group.py
all_test_affinity_groups Skipped --- test_affinity_groups.py
all_test_internal_lb Skipped --- test_internal_lb.py
all_test_usage Skipped --- test_usage.py
all_test_affinity_groups_projects Skipped --- test_affinity_groups_projects.py
all_test_register_userdata Skipped --- test_register_userdata.py
all_test_annotations Skipped --- test_annotations.py
all_test_reset_vm_on_reboot Skipped --- test_reset_vm_on_reboot.py
all_test_async_job Skipped --- test_async_job.py
all_test_ipv6_infra Skipped --- test_ipv6_infra.py
all_test_attach_multiple_volumes Skipped --- test_attach_multiple_volumes.py
all_test_kubernetes_clusters Skipped --- test_kubernetes_clusters.py
all_test_backup_recovery_dummy Skipped --- test_backup_recovery_dummy.py
all_test_outofbandmanagement Skipped --- test_outofbandmanagement.py
all_test_certauthority_root Skipped --- test_certauthority_root.py
all_test_over_provisioning Skipped --- test_over_provisioning.py
all_test_console_endpoint Skipped --- test_console_endpoint.py
all_test_set_sourcenat Skipped --- test_set_sourcenat.py
all_test_create_list_domain_account_project Skipped --- test_create_list_domain_account_project.py
all_test_password_server Skipped --- test_password_server.py
all_test_create_network Skipped --- test_create_network.py
all_test_iso Skipped --- test_iso.py
all_test_snapshots Skipped --- test_snapshots.py
all_test_deploy_vgpu_enabled_vm Skipped --- test_deploy_vgpu_enabled_vm.py
all_test_migration Skipped --- test_migration.py
all_test_deploy_virtio_scsi_vm Skipped --- test_deploy_virtio_scsi_vm.py
all_test_multipleips_per_nic Skipped --- test_multipleips_per_nic.py
all_test_deploy_vm_extra_config_data Skipped --- test_deploy_vm_extra_config_data.py
all_test_persistent_network Skipped --- test_persistent_network.py
all_test_deploy_vm_iso Skipped --- test_deploy_vm_iso.py
all_test_portable_publicip Skipped --- test_portable_publicip.py
all_test_deploy_vm_iso_uefi Skipped --- test_deploy_vm_iso_uefi.py
all_test_nested_virtualization Skipped --- test_nested_virtualization.py
all_test_deploy_vm_root_resize Skipped --- test_deploy_vm_root_resize.py
all_test_network Skipped --- test_network.py
all_test_deploy_vm_with_userdata Skipped --- test_deploy_vm_with_userdata.py
all_test_network_acl Skipped --- test_network_acl.py
all_test_deploy_vms_in_parallel Skipped --- test_deploy_vms_in_parallel.py
all_test_ssvm Skipped --- test_ssvm.py
all_test_deploy_vms_with_varied_deploymentplanners Skipped --- test_deploy_vms_with_varied_deploymentplanners.py
all_test_resource_accounting Skipped --- test_resource_accounting.py
all_test_diagnostics Skipped --- test_diagnostics.py
all_test_portforwardingrules Skipped --- test_portforwardingrules.py
all_test_direct_download Skipped --- test_direct_download.py
all_test_primary_storage Skipped --- test_primary_storage.py
all_test_disk_offerings Skipped --- test_disk_offerings.py
all_test_network_ipv6 Skipped --- test_network_ipv6.py
all_test_disk_provisioning_types Skipped --- test_disk_provisioning_types.py
all_test_network_permissions Skipped --- test_network_permissions.py
all_test_domain_disk_offerings Skipped --- test_domain_disk_offerings.py
all_test_nic Skipped --- test_nic.py
all_test_staticroles Skipped --- test_staticroles.py
all_test_domain_network_offerings Skipped --- test_domain_network_offerings.py
all_test_nic_adapter_type Skipped --- test_nic_adapter_type.py
all_test_domain_service_offerings Skipped --- test_domain_service_offerings.py
all_test_private_roles Skipped --- test_private_roles.py
all_test_domain_vpc_offerings Skipped --- test_domain_vpc_offerings.py
all_test_resource_detail Skipped --- test_resource_detail.py
all_test_dynamicroles Skipped --- test_dynamicroles.py
all_test_hostha_simulator Skipped --- test_hostha_simulator.py
all_test_enable_account_settings_for_domain Skipped --- test_enable_account_settings_for_domain.py
all_test_human_readable_logs Skipped --- test_human_readable_logs.py
all_test_enable_role_based_users_in_projects Skipped --- test_enable_role_based_users_in_projects.py
all_test_privategw_acl Skipped --- test_privategw_acl.py
all_test_events_resource Skipped --- test_events_resource.py
all_test_non_contigiousvlan Skipped --- test_non_contigiousvlan.py
all_test_gateway_on_shared_networks Skipped --- test_gateway_on_shared_networks.py
all_test_privategw_acl_ovs_gre Skipped --- test_privategw_acl_ovs_gre.py
all_test_global_settings Skipped --- test_global_settings.py
all_test_router_dhcphosts Skipped --- test_router_dhcphosts.py
all_test_guest_os Skipped --- test_guest_os.py
all_test_projects Skipped --- test_projects.py
all_test_guest_vlan_range Skipped --- test_guest_vlan_range.py
all_test_public_ip_range Skipped --- test_public_ip_range.py
all_test_host_control_state Skipped --- test_host_control_state.py
all_test_kubernetes_supported_versions Skipped --- test_kubernetes_supported_versions.py
all_test_pvlan Skipped --- test_pvlan.py
all_test_list_ids_parameter Skipped --- test_list_ids_parameter.py
all_test_router_dns Skipped --- test_router_dns.py
all_test_loadbalance Skipped --- test_loadbalance.py
all_test_router_dnsservice Skipped --- test_router_dnsservice.py
all_test_login Skipped --- test_login.py
all_test_routers Skipped --- test_routers.py
all_test_metrics_api Skipped --- test_metrics_api.py
all_test_routers_network_ops Skipped --- test_routers_network_ops.py
all_test_outofbandmanagement_nestedplugin Skipped --- test_outofbandmanagement_nestedplugin.py
all_test_reset_configuration_settings Skipped --- test_reset_configuration_settings.py
all_test_routers_iptables_default_policy Skipped --- test_routers_iptables_default_policy.py
all_test_secondary_storage Skipped --- test_secondary_storage.py
all_test_service_offerings Skipped --- test_service_offerings.py
all_test_storage_policy Skipped --- test_storage_policy.py
all_test_templates Skipped --- test_templates.py
all_test_update_security_group Skipped --- test_update_security_group.py
all_test_usage_events Skipped --- test_usage_events.py
all_test_vm_autoscaling Skipped --- test_vm_autoscaling.py
all_test_vm_deployment_planner Skipped --- test_vm_deployment_planner.py
all_test_vm_life_cycle Skipped --- test_vm_life_cycle.py
all_test_vm_lifecycle_unmanage_import Skipped --- test_vm_lifecycle_unmanage_import.py
all_test_vm_schedule Skipped --- test_vm_schedule.py
all_test_vm_snapshot_kvm Skipped --- test_vm_snapshot_kvm.py
all_test_vm_snapshots Skipped --- test_vm_snapshots.py
all_test_volumes Skipped --- test_volumes.py
all_test_vpc_ipv6 Skipped --- test_vpc_ipv6.py
all_test_vpc_redundant Skipped --- test_vpc_redundant.py
all_test_vpc_router_nics Skipped --- test_vpc_router_nics.py
all_test_vpc_vpn Skipped --- test_vpc_vpn.py
all_test_host_maintenance Skipped --- test_host_maintenance.py
all_test_hostha_kvm Skipped --- test_hostha_kvm.py

@blueorangutan
Copy link

[SF] Trillian test result (tid-7110)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46064 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7088-t7110-kvm-centos7.zip
Smoke tests completed. 111 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_migrate_VM_and_root_volume Error 81.64 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 56.40 test_vm_life_cycle.py
test_02_redundant_VPC_default_routes Failure 370.32 test_vpc_redundant.py

@DaanHoogland
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@DaanHoogland
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 6548

@DaanHoogland
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6549

@DaanHoogland
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-7148)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41002 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7088-t7148-kvm-centos7.zip
Smoke tests completed. 112 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_migrate_VM_and_root_volume Error 79.74 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 52.41 test_vm_life_cycle.py
test_01_secure_vm_migration Error 151.20 test_vm_life_cycle.py

Copy link
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code change look okay

@DaanHoogland
Copy link
Contributor Author

let's test again
@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 7210

@DaanHoogland
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Copy link
Member

@vishesh92 vishesh92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code lgtm

@blueorangutan
Copy link

[SF] Trillian test result (tid-7824)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48470 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7088-t7824-kvm-centos7.zip
Smoke tests completed. 112 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_05_vmschedule_test_e2e Failure 270.98 test_vm_schedule.py

@DaanHoogland DaanHoogland merged commit e21b888 into apache:main Oct 4, 2023
50 of 52 checks passed
@DaanHoogland DaanHoogland deleted the errorProne branch October 6, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants