Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Add test cases for the validation of IAM permissions when creating a cloud service provider
Comment | File | Size | Author |
---|---|---|---|
#19 | 3202937-19.patch | 47.17 KB | kumikoono |
Issue fork cloud-3202937
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:
- 3202937-1 changes, plain diff MR !186
- 3202937-add-test-cases changes, plain diff MR !179
Comments
Comment #2
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commentedComment #4
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commented@yas Can you review this patch?
Comment #5
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commentedRemoved unnecessary changes.
Comment #6
yas@kumikoono
Thank you for adding the test cases. Please see my above comments. Thanks
Comment #7
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commented@yas Thanks for your review.
Incorporated your comments. Regarding refactoring
addMockHandler(),
I also refactoredOpenStackService.php.
Comment #8
baldwinlouie CreditAttribution: baldwinlouie commented@kumikoono @yas, the patch looks good to me, except one small comment below.
paraeter is mispelled
Comment #9
yas@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
Comment #10
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commented@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.Comment #11
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commentedComment #12
yas@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.Comment #13
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commented@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.Comment #14
yas@kunikoono
Thank you for the update. I posted my comments. Please check.
Comment #15
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commentedComment #16
yas@kumikoono
Thank you for the update. I have my above comment. Let's reduce `else` basically. Thanks.
Comment #19
kumikoono CreditAttribution: kumikoono at DOCOMO Innovations, Inc. commented- Resolved merge conflicts
- Removed 'else' from
Ec2Service
andOpenStackService
to incorporate your comment- Removed '0' from the argument of
new DateTimeResult()
inOpenStackService::addMockHandler()
Comment #20
yas@kumikoono
Thank you for the update. It looks good to me now. I'll merge the patch to
3.x
and close this issue asFixed
.Comment #23
yas