Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yas created an issue. See original summary.

yas’s picture

FileSize
103.33 KB
  • Added the test cases:
    • CloudModuleTestBase
    • AwsCloudInstallUninstallTest
    • CloudBudgetInstallUninstallTest
    • K8sInstallUninstallTest
    • OpenStackInstallUninstallTest
    • CloudInstallUninstallTest
  • Added hook_help for the modules:
    • cloud
    • aws_cloud
    • k8s
    • openstack
yas’s picture

Status: Needs work » Needs review
FileSize
103.29 KB

Fixing the coding standard violation.

yas’s picture

baldwinlouie’s picture

Status: Needs review » Needs work

@yas, I have the following comments about this patch.

  1. +++ b/cloud.module
    @@ -180,6 +184,27 @@ function cloud_uninstall_location_fields($module_name, $bundle = NULL) {
    +  // Delete default icon.
    +  $config_factory = \Drupal::configFactory();
    +  $config = $config_factory->getEditable("${module}.settings");
    +  $fid = $config->get("${module}_cloud_config_icon");
    +
    +  // Delete file from disk and from database.
    +  if (!empty($fid)) {
    +    $storage = \Drupal::entityTypeManager()->getStorage('file');
    +    $files = $storage->loadMultiple([$fid]);
    +    $storage->delete($files);
    +  }
    

    Does this function need a try/catch block?

  2. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.install
    @@ -36,6 +36,9 @@ function aws_cloud_uninstall() {
    @@ -66,7 +69,7 @@ function aws_cloud_add_default_icon() {
    
    @@ -66,7 +69,7 @@ function aws_cloud_add_default_icon() {
         $file->save();
         $config_factory = \Drupal::configFactory();
         $config = $config_factory->getEditable('aws_cloud.settings');
    -    $config->set('aws_cloud_config_icon', $file->id());
    +    $config->set('aws_cloud_cloud_config_icon', $file->id());
         $config->save();
       }
    

    This function might change depending on the status of https://www.drupal.org/project/cloud/issues/3120163

  3. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.install
    @@ -2394,3 +2397,12 @@ function aws_cloud_update_8218() {
    +function aws_cloud_update_8219() {
    +
    +  // Creates 'aws_cloud_cloud_config_icon'.
    +  aws_cloud_add_default_icon();
    +}
    

    This might need to change depending on
    https://www.drupal.org/project/cloud/issues/3120163

  4. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.module
    @@ -4155,7 +4184,7 @@ function aws_cloud_cloud_config_fieldsets(array &$fields) {
    -  $fields['#attached']['library'][] = 'aws_cloud/aws_cloud_config';
    +  $fields['#attached']['library'][] = 'aws_cloud/aws_cloud_cloud_config';
    

    aws_cloud_config js library has been removed. I don't know if this attach is needed anymore.

yas’s picture

FileSize
103.4 KB
1.75 KB

@baldwinlouie

Thank you for your review. I just updated the patch to fix 1. and 4. I didn't fix 2. 3.

yas’s picture

Status: Needs work » Needs review
FileSize
103.33 KB

@baldwinlouie

I re-created the patch after merging #3120163. #3115078 and #3119979. Could you please review it again?

yas’s picture

The test result:

52.png

yas’s picture

baldwinlouie’s picture

Status: Needs review » Needs work

@yas, Thank you for the updated patch. I have the following comments.

  1. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.install
    @@ -2369,3 +2372,12 @@ function aws_cloud_update_8218() {
    +}
    

    K8s and OpenStack should have an update function that updates the default icon.

  2. +++ b/modules/cloud_service_providers/k8s/k8s.install
    @@ -1571,8 +1576,23 @@ function k8s_update_8272() {
    +  // Obtain the storage manager for field termination protection bases
    

    `field termination` should be `field_launch_resources`

yas’s picture

@baldwinlouie

Thank for your review.

  1. We don't have to update the default icon of both K8s and OpenStack since those are no change for the filename such as k8s.png and openstack.png. I aligned the default icon filename is equals to module name. Therefore for aws_cloud, I changed the icon name from aws.png to aws_cloud.png; that's why I put aws_cloud_update_8219 to address this change.
  2. I fixed it. Thanks.

Moreover, I added the install/uninstall test for the other modules such as

  • Drupal\Tests\docker\Functional\Module\DockerInstallUninstallTest
  • Drupal\Tests\gapps\Functional\Module\GAppsInstallUninstallTest
  • Drupal\Tests\k8s_to_s3\Functional\Module\K8sToS3InstallUninstallTest
  • Drupal\Tests\s3_to_k8s\Functional\Module\S3ToK8sInstallUninstallTest

Here is the test results:

14.png

Could you please review it?

yas’s picture

Fixed the coding standard violations.

baldwinlouie’s picture

Status: Needs review » Reviewed & tested by the community

@yas, thank you for the updated patch. It looks good to me now.

yas’s picture

@baldwinlouie

Thank you for your review. I'll merge the patch to 8.x-1.x and 8.x-2.x and close this issue as Fixed.

  • yas committed a30b5b3 on 8.x-1.x
    Issue #3119973 by yas, baldwinlouie: Add test cases to install /...

  • yas committed aaf0fab on 8.x-2.x
    Issue #3119973 by yas, baldwinlouie: Add test cases to install /...
yas’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.