CommentFileSizeAuthor
#14 duplicate_setDomain-3014213-14.patch744 bytesXiaohua Guan
#7 3014213-7.patch3.88 KBXiaohua Guan
#2 3014213_20181116.patch1.51 KBXiaohua Guan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xiaohua Guan created an issue. See original summary.

Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented
FileSize
3014213_20181116.patch1.51 KB
Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented
Status: Active » Needs review
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
CreditAttribution: yas as a volunteer and at DOCOMO Innovations, Inc. commented

@baldwinlouie It looks good to me but could you please review it, too?

baldwinlouie’s picture

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

@yas, this looks good.

baldwinlouie’s picture

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

@yas, and @xiaohua-guan. Sorry, but I'm setting this back to needs work.

After looking at this again, I think it might be better to have helper methods in ElasticIp.php such as set_network_interface_id(), set_association_id(). The patch above will then call these methods instead of directly setting the value of the entity.

What do you think?

Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented
FileSize
3014213-7.patch3.88 KB
Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented
Status: Needs work » Needs review
Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented

@baldwinlouie

Thanks for your review.

I've added helper functions to the entity class. Please take a look again.

Thanks.

baldwinlouie’s picture

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

Thanks for the updated patch. Looks good!

  • yas committed 129ef14 on 8.x-1.x authored by Xiaohua Guan 
    Issue #3014213 by Xiaohua Guan, baldwinlouie, yas: Update Elastic IP...
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
CreditAttribution: yas as a volunteer and at DOCOMO Innovations, Inc. commented
Status: Reviewed & tested by the community » Fixed

@xiaohua-guan
@baldwinlouie

Thank you for your contribution. I merged the patch.

Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented
Status: Fixed » Needs work
Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented
FileSize
duplicate_setDomain-3014213-14.patch744 bytes
Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented
Status: Needs work » Needs review
Xiaohua Guan’s picture

Xiaohua Guan CreditAttribution: Xiaohua Guan commented

@yas

There is a critical problem in ElasticIp.php, where the method setDomain was defined duplicately.

Please review the patch file duplicate_setDomain-3014213-14.patch.

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
CreditAttribution: yas at DOCOMO Innovations, Inc. commented
Status: Needs review » Reviewed & tested by the community

@xiaohua-guan

Thank you for fixing it. It looks good to me. I'll push and merge it.

  • yas committed d94bf86 on 8.x-1.x authored by Xiaohua Guan 
    Issue #3014213 by Xiaohua Guan, yas, baldwinlouie: Update Elastic IP...
yas’s picture

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

Status: Fixed » Closed (fixed)

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