Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow advanced modifications of Amazon cloud volumes #676

Merged
merged 1 commit into from Mar 24, 2017

Conversation

gberginc
Copy link
Contributor

Amazon EBS has recently introduced ability to modify cloud volume
configuration (type of cloud volume, its size and the IOPS). This patch
reuses the UI from the cloud volume creation dialog to allow users to
modify volumes on the fly.

Depends on

Video: http://x.k00.fr/7t19f

  • shows how different volume types are created
  • compares OpenStack and Amazon UIs
  • shows UI for editing standard (Magnetic), gp2 and io1 volumes
    • magnetic: only name is enabled, other info just shown
    • gp2: type and size can be changed; IOPS disabled
    • io1: all fields can be changed
  • video shows AWS console to present the results

@miq-bot add_label enhancement, storage

@gberginc
Copy link
Contributor Author

@AparnaKarve this is the form I talked about yesterday. Although it depends on the Amazon provider changes, I would appreciate any feedback.

@@ -14,7 +14,7 @@ ManageIQ.angular.app.controller('cloudVolumeFormController', ['$http', '$scope',
{ type: "io1", name: "Provisioned IOPS SSD (IO1)" },
{ type: "st1", name: "Throughput Optimized HDD (ST1)" },
{ type: "sc1", name: "Cold HDD (SC1)" },
{ type: "magnetic", name: "Magnetic" },
{ type: "standard", name: "Magnetic" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would have really liked if we could fetch this list on-the-fly using some API (AWS API perhaps?)
But don't worry about it, if that's not doable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWS does not provide an API for this; we initially thought about making CloudVolumeType model where storage managers would be able to list their options (for example in OpenStack admins are allowed to define different volume types so it would make sense).

My concern is that there are specific rules in the UI that need to be handled with regards to the chosen volume type, so I am not sure if providing it through API/backend would benefit much. Still, we could use something like this for regions or instance types. Would that make any sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, yes. I know you mentioned this even before.

@@ -0,0 +1,60 @@
.form-group{"ng-class" => "{'has-error': angularForm.name.$invalid}"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this haml to _form.html.haml?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I missed that completely.

'ng-maxlength' => 128,
:miqrequired => false,
:checkchange => true}
= render :partial => "common_new_edit"

%table{:width => '100%'}
%tr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this haml, down below, can you use this partial to render the buttons?
"layouts/angular/x_edit_buttons_angular"
instead of rendering the buttons individually.
(I probably missed telling you this in your earlier PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure - I didn't know about this before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AparnaKarve I noticed there are some differences between the way how the button table is created for "new volume" than in the about partial layout. For example, in cloud volume case it checks if @volume.id.nil? to decide whether to show Add or Save, while the layout partial uses newRecord.

I guess we should update the whole controller/view to make it compliant with the partial layout. Perhaps in a separate PR. What do you think?

:checkchange => true}
%span.help-block{"ng-show" => "cloudVolumeModel.aws_volume_type == 'io1' && angularForm.aws_iops.$error.required"}
= _("Required")
= render :partial => "common_new_edit"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above for the button partial

@AparnaKarve
Copy link
Contributor

@gberginc I have tested this in the UI and it works great!
Overall, this looks good other than some of the few minor changes mentioned above. Thanks.

$scope.cloudVolumeModel.size = data.size;
// Currently, this is only relevant for AWS volumes so we are prefixing the
// model attribute with AWS.
$scope.cloudVolumeModel.aws_volume_type = data.volume_type;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gberginc We now have REST API support for the cloud_volumes collection...which means we can get rid of this $http.get call here and replace it with --

/api/cloud_volumes/<id>?attributes=ext_management_system.type

Amazon EBS has recently introduced ability to modify cloud volume
configuration (type of cloud volume, its size and the IOPS). This patch
reuses the UI from the cloud volume creation dialog to allow users to
modify volumes on the fly.
@gberginc
Copy link
Contributor Author

@AparnaKarve I've renamed the haml file, replaced $http with API and removed the corresponding call in the controller (+ route) since it's not used elsewhere.

I am not sure about the buttons at the bottom if it makes sense to change it within this PR, so I'd appreciate your feedback.

@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2017

Checked commit gberginc@b3527cc with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 1 offense detected

app/views/cloud_volume/_common_new_edit.html.haml

  • ⚠️ - Line 41 - Line is too long. [194/160]

@gberginc gberginc closed this Mar 16, 2017
@gberginc gberginc reopened this Mar 16, 2017
@AparnaKarve
Copy link
Contributor

replaced $http with API and removed the corresponding call in the controller (+ route) since it's not used elsewhere.

Awesome! I reviewed that part and it looks good.

I am not sure about the buttons at the bottom if it makes sense to change it within this PR

A separate follow-up PR later should be fine. Not a problem.

@gberginc
Copy link
Contributor Author

@AparnaKarve is there anything more to change in this one?

@AparnaKarve
Copy link
Contributor

@gberginc I think everything is addressed here other than the buttons, but there will be a follow-up PR for that.

@h-kataria Can you please merge this?

@h-kataria h-kataria added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 24, 2017
@h-kataria h-kataria merged commit 18ef11c into ManageIQ:master Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants