Problem/Motivation

Refactor permission and fresh message structure to make it as same logic as k8s module

CommentFileSizeAuthor
#11 3164220-11.patch9.89 KBliuchanggang
#8 3164220-8.patch10.25 KBliuchanggang
#4 3164220-4.patch42.73 KBliuchanggang
#2 3164220-2.patch40.05 KBliuchanggang

Comments

liuchanggang created an issue. See original summary.

liuchanggang’s picture

liuchanggang commented
Status: Active » Needs review
StatusFileSize
new 3164220-2.patch40.05 KB

I updated route permission and refactored messages after update resources.

Status: Needs review » Needs work

The last submitted patch, 2: 3164220-2.patch, failed testing. View results

liuchanggang’s picture

liuchanggang commented
Status: Needs work » Needs review
StatusFileSize
new 3164220-4.patch42.73 KB
liuchanggang’s picture

liuchanggang commented

We can hold this issue till 3163932 is done.

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented

@liuchanggang

Thank you for understanding.

@jigishaddweb

We'll wait for the patch at #3163932.

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Related issues: +#3163932: Fix a bug in Ec2Service::updateInstanceEntities()
liuchanggang’s picture

liuchanggang commented
StatusFileSize
new 3164220-8.patch10.25 KB

Code became much simpler after rebased latest 3.x changes.
Remaining changes are on updating message after refreshing
This is ready for review.

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented

@liuchanggang

Thank you for the update. It looks good to me now.

@baldwinlouie

What do you think?

baldwinlouie’s picture

baldwinlouie commented
Status: Needs review » Needs work

@liuchanggang, Thank you for the patch. It looks pretty good. I have the following comment

+++ b/modules/cloud_service_providers/aws_cloud/src/Controller/Ec2/ApiController.php
@@ -349,7 +344,7 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
+    return $this->ec2Service->updateAllResourceList('aws_cloud_elastic_ip', '');

What is the second '' parameter for?

Is that parameter needed?

liuchanggang’s picture

liuchanggang commented
Status: Needs work » Needs review
StatusFileSize
new 3164220-11.patch9.89 KB

@baldwinlouie

Thanks for review. I have fixed that line.

baldwinlouie’s picture

baldwinlouie commented
Status: Needs review » Reviewed & tested by the community

@liuchanggang

Thank you for the patch. It looks good to me now.

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented

@baldwinlouie

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

  • yas committed 8d36a1a on 8.x-1.x authored by liuchanggang 
    Issue #3164220 by liuchanggang, yas, baldwinlouie: Refactor the Refresh...

  • yas committed e9173a5 on 8.x-2.x authored by liuchanggang 
    Issue #3164220 by liuchanggang, yas, baldwinlouie: Refactor the Refresh...

  • yas committed 3ace989 on 3.x authored by liuchanggang 
    Issue #3164220 by liuchanggang, yas, baldwinlouie: Refactor the Refresh...
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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