CommentFileSizeAuthor
#7 interdiff-3166437-7.txt791 bytesjigish.addweb
#7 3166437-7.patch32.83 KBjigish.addweb
#3 interdiff-3166437-3.patch2.27 KBjigish.addweb
#3 3166437-3.patch32.55 KBjigish.addweb
#2 3166437-2.patch30.48 KBjigish.addweb

Comments

jigish.addweb created an issue. See original summary.

jigish.addweb’s picture

jigish.addweb commented
Issue summary: View changes
StatusFileSize
new 3166437-2.patch30.48 KB
jigish.addweb’s picture

jigish.addweb commented
StatusFileSize
new 3166437-3.patch32.55 KB
new interdiff-3166437-3.patch2.27 KB
jigish.addweb’s picture

jigish.addweb commented
Status: Active » Needs review
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Title: Refactor the refresh logic for all listing view of OpenStack » Refactor the Refresh logic in All listing view in each OpenStack resource (2)
Related issues: +#3164220: Refactor the Refresh logic in All listing view in each AWS resource, +#3164269: Refactor the Refresh logic in All listing view in each OpenStack resource

@jigishaddweb

Thank you for the refactoring.

@liuchanggang

Could you please review the patch?

liuchanggang’s picture

liuchanggang commented
Status: Needs review » Needs work

The code looks good. However, I see following potential issues.
Some updateAll***() functions were removed from service, but they are still called by cron jobs. Please make sure it will not break those cron jobs. 

  public function createResourceQueueItems() {
    $update_resources_queue = $this->queueFactory->get('openstack_update_resources_queue');
    $method_names = [
      'terminateExpiredInstances',
      'updateInstances',
      'updateImages',
      'updateKeyPairs',
      'updateSecurityGroups',
      'updateVolumes',
      'updateSnapshots',
      'updateNetworkInterfaces',
      'updateFloatingIps',
      'updatePendingImages',
      'updateAllInstances',
      'updateAllImages',
      'updateAllSecurityGroups',
      'updateAllFloatingIp',
      'updateAllNetworkInterfaces',
      'updateAllKeyPairs',
      'updateAllVolumes',
      'updateAllSnapshots',
    ];

Thanks.

jigish.addweb’s picture

jigish.addweb commented
StatusFileSize
new 3166437-7.patch32.83 KB
new interdiff-3166437-7.txt791 bytes
jigish.addweb’s picture

jigish.addweb commented
Status: Needs work » Needs review

@yas @liuchanggang

Thank you for your review.

I removed updateAll* methods from createResourceQueueItems.

Please review the updated patch.

Thanks

liuchanggang’s picture

liuchanggang commented

@jigish.addweb.
Thanks for updating. The code looks good to me now.

liuchanggang’s picture

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

Changed to status to be RTBC. Thanks

yas’s picture

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

@liuchanggang

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 efeba4c on 8.x-2.x authored by jigish.addweb 
    Issue #3166437 by jigish.addweb, liuchanggang, yas: Refactor the Refresh...

  • yas committed c8da2a4 on 3.x authored by jigish.addweb 
    Issue #3166437 by jigish.addweb, liuchanggang, yas: Refactor the Refresh...

  • yas committed 46c04ac on 8.x-1.x authored by jigish.addweb 
    Issue #3166437 by jigish.addweb, liuchanggang, yas: 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.