-
Notifications
You must be signed in to change notification settings - Fork 358
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
Network router gateway setup #716
Network router gateway setup #716
Conversation
👍 from a functionality perspective; calls the API correctly. Double-checking the code, this doesn't actually have any gem changes, correct? |
@@ -66,12 +68,25 @@ ManageIQ.angular.app.controller('networkRouterFormController', ['$http', '$scope | |||
.catch(miqService.handleFailure); | |||
}; | |||
|
|||
$scope.filterCloudNetworkChanged = function(id) { | |||
miqService.sparkleOn(); | |||
$http.get('/network_router/network_router_subnets_by_network/' + id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gildub I see that you have added a new controller route and action -- network_router_subnets_by_network
Currently we have a new practice in place for all our angular forms, which is --
Use REST API calls and avoid creating new controller actions/routes, especially true for any new functionality being added in the forms.
Hence I recommend using an API call here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked that currently an API for network_routers
does not exist, but it should not be too hard to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the fact that this is an RFE BZ for 5.7... so maybe the REST API implementation can wait for now.
Can you create a follow-up PR later to call the REST API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gildub Thanks for creating the issue. We can work on this once this PR is merged.
'ng-maxlength' => 128, | ||
:miqrequired => true, | ||
:checkchange => true} | ||
%h3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fields that are Required need to be explicitly shown as Required
including the dropdowns below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AparnaKarve , all the router gateway related field are optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gildub Some fields are Required
, because otherwise the Add
button will not light up (turn blue, that is) as seen in the screenshots below --
We need to clearly say Required
for every Required field, it just makes for a better UX, that's all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, yes the name is required and that one was removed by mistake, thanks
@tzumainn I think saving the Network Router record should be a queued operation (if it isn't already), otherwise the spinner might keep spinning indefinitely during a Save. |
@AparnaKarve I think it's a Task - @gildub, is that correct? I think optimally this makes it into the next 5.7 build, so if we can implement the REST API bit immediately after that would be great! |
@gildub screen shots in the first comment would be appreciated 😄 |
@dclarizio, I added the screen shot, sorry I didn't have it in the first place, that was my intent but 'cause of Murphy's law I couldn't launch my instance anymore, Friday night, blah-blah :) Please let me know if you think more information is required. @AparnaKarve, please see my inline comment about API. @tzumainn -Effectively no Gem added and using a task queue, nothing changed there. Thanks |
Checked commits https://github.com/gildub/manageiq-ui-classic/compare/679cb360fe7e7b8f180cef1a8e27532ac18b23f3~...7b2aff199a36f5975de953fbd0cf37a29e9a849c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/controllers/network_router_controller.rb
|
@gildub Thanks for addressing all the feedback! |
@gildub There is a conflict backporting to Euwe. Please resolve conflict and create an Euwe-specific PR (referencing this one) or suggest other PRs to backport.
|
Backported to Euwe via ManageIQ/manageiq#14447 (Mar 27) |
The external gateway information wasn't fully handled.
Now external gateway values are correctly set up allowing to add reset ("clear") the gateway.
https://bugzilla.redhat.com/show_bug.cgi?id=1426268
Edit and Create share the same template: