Problem/Motivation

Add test cases for the validation of IAM permissions when creating a cloud service provider

Issue fork cloud-3202937

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kumikoono created an issue. See original summary.

kumikoono’s picture

kumikoono’s picture

Status: Active » Needs review
FileSize
40.03 KB

@yas Can you review this patch?

kumikoono’s picture

FileSize
37.91 KB

Removed unnecessary changes.

yas’s picture

Status: Needs review » Needs work

@kumikoono

Thank you for adding the test cases. Please see my above comments. Thanks

kumikoono’s picture

Status: Needs work » Needs review
FileSize
47.75 KB

@yas Thanks for your review.
Incorporated your comments. Regarding refactoring addMockHandler(), I also refactored OpenStackService.php.

baldwinlouie’s picture

Status: Needs review » Needs work

@kumikoono @yas, the patch looks good to me, except one small comment below.

+++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
@@ -308,47 +309,55 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+      // Checking the value of DryRun paraeter as some test cases expect a

paraeter is mispelled

yas’s picture

@baldwinlouie

Thank you for your review.

@kumikoono

Thank you for the update. I put my comments. Also, could you please add more comments as I suggested in my previous review in the closure of each addMockHandler()?

Thanks

kumikoono’s picture

FileSize
48 KB

@yas @baldwin, thanks for your review.
Can you review the discussion on
https://git.drupalcode.org/project/cloud/-/merge_requests/179#note_16316 ?

I'll add comments into addMockeHandler() once the logic is clarified and if any part is still not self-explaining.

kumikoono’s picture

Status: Needs work » Needs review
yas’s picture

Status: Needs review » Needs work

@kumikoono

Thank you for the update. There are some leftovers as I previously commented. Also, could you please add more comments to elaborate the logic in as I suggested in the closure of each addMockHandler()? Please refer to my previous suggested code.

kumikoono’s picture

Status: Needs work » Needs review
FileSize
47.14 KB

@yas Thanks for your review. Incorporated all your comments.
Regarding addMockHandler(), the logic is now much simpler. Comments are added but limited to non-self-explaining logic.

yas’s picture

Status: Needs review » Needs work

@kunikoono

Thank you for the update. I posted my comments. Please check.

kumikoono’s picture

Status: Needs work » Needs review
FileSize
47.17 KB
yas’s picture

Status: Needs review » Needs work

@kumikoono

Thank you for the update. I have my above comment. Let's reduce `else` basically. Thanks.

kumikoono’s picture

Status: Needs work » Needs review
FileSize
47.17 KB

- Resolved merge conflicts
- Removed 'else' from Ec2Service and OpenStackService to incorporate your comment
- Removed '0' from the argument of new DateTimeResult() in OpenStackService::addMockHandler()

yas’s picture

Status: Needs review » Reviewed & tested by the community

@kumikoono

Thank you for the update. It looks good to me now. I'll merge the patch to 3.x and close this issue as Fixed.

  • yas committed a738b7b on 3.x authored by kumikoono
    Issue #3202937 by kumikoono, yas, baldwinlouie: Add test cases for the...

yas’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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