Add a way to attach and detach volumes from instances.

CommentFileSizeAuthor
#9 30140911_3.patch20.09 KBbaldwinlouie
#7 30140911_2.patch17.79 KBbaldwinlouie
#6 30140911_1.patch15.04 KBbaldwinlouie
#2 3014091.patch19.02 KBbaldwinlouie

Comments

baldwinlouie created an issue. See original summary.

baldwinlouie’s picture

baldwinlouie commented
StatusFileSize
new 3014091.patch19.02 KB

Attaching patch that adds this functionality. This adds an "Attach" and "Detach" link to the Volume Operations.

baldwinlouie’s picture

baldwinlouie commented
Status: Active » Needs review

Attaching patch that adds this functionality. This adds an "Attach" and "Detach" link to the Volume Operations.

Status: Needs review » Needs work

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

yas’s picture

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

@baldwinlouie

Thank you for the patch. It contains the path: modules/contrib/, therefore the automated test didn't pass...

error: modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Entity/Ec2/Volume.php: No such file or directory
error: modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/aws_cloud.routing.yml: No such file or directory
error: modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Service/AwsEc2ServiceInterface.php: No such file or directory
error: modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/config/install/views.view.aws_volume.yml: No such file or directory
error: modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Form/Ec2/AwsDeleteForm.php: No such file or directory
error: modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/aws_cloud.module: No such file or directory
error: modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Service/AwsEc2Service.php: No such file or directory
baldwinlouie’s picture

baldwinlouie commented
StatusFileSize
new 30140911_1.patch15.04 KB

Rerolling the patch. This time from the command line instead of from phpstorm.

baldwinlouie’s picture

baldwinlouie commented
StatusFileSize
new 30140911_2.patch17.79 KB

rerolling again. This time with git diff HEAD to properly recognize the newly added files.

yas’s picture

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

@baldwinlouie
@xiaohua-guan

It looks having an issue for AwsDeleteForm?

1) Drupal\Tests\aws_cloud\Functional\Ec2\ImageTest::testImage
Exception: TypeError: Argument 5 passed to Drupal\aws_cloud\Form\Ec2\AwsDeleteForm::__construct() must implement interface Drupal\Core\Entity\EntityTypeManagerInterface, none given, called in /var/www/html/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Form/Ec2/ImageDeleteForm.php on line 59
Drupal\aws_cloud\Form\Ec2\AwsDeleteForm->__construct()() (Line: 40)

2) Drupal\Tests\aws_cloud\Functional\Ec2\ImageTest::testImportImage
Exception: TypeError: Argument 5 passed to Drupal\aws_cloud\Form\Ec2\AwsDeleteForm::__construct() must implement interface Drupal\Core\Entity\EntityTypeManagerInterface, none given, called in /var/www/html/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Form/Ec2/ImageDeleteForm.php on line 59
Drupal\aws_cloud\Form\Ec2\AwsDeleteForm->__construct()() (Line: 40)
baldwinlouie’s picture

baldwinlouie commented
StatusFileSize
new 30140911_3.patch20.09 KB

@yas, Rerolling the patch. I added the EntityTypeManagerInterface to the ImageDeleteForm. I added this parameter to AWSDeleteForm so that entity query is available for all delete forms.

I think we should rename AWSDeleteForm in the future. It has become more of a confirmation form. I use it when starting/stopping instances, attaching/detaching volumes. It should become something like AWSConfirmationForm.

baldwinlouie’s picture

baldwinlouie commented
Status: Needs work » Needs review
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Status: Needs review » Needs work

@baldwinlouie

Thank you for your modification. I tested your patch by myself, and now it looks good to me with the successful automated test.

I think we need the test code for that function. Also, you can rename AwsDeleteFormto AwsConfirmationForm in your patch? But in that case, I wonder if a developer would be confused about the classes without AwsDeleteForm, which only contain AwsConfirmForm.

@xiaohua-guan

Could you please review the patch?

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Status: Needs work » Needs review

We will discuss to include test code in another issue.

xiaohua guan’s picture

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

@yas

> Could you please review the patch?
I reviewed. It looks ok to me. And I changed the status to RTBC.

  • yas committed 82ade4e on 8.x-1.x authored by baldwinlouie 
    Issue #3014091 by baldwinlouie, yas, Xiaohua Guan: Add Attach/Detach...
yas’s picture

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

@baldwinlouie
@xiaohua-guan

Thank you for your contribution. I pushed and merged the patch.

Status: Fixed » Closed (fixed)

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