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.

Sign up for GitHub

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

Jump to bottom

Add Smart Jdbc driver defaults #876

Merged
merged 2 commits into from
Apr 5, 2017
Merged

Add Smart Jdbc driver defaults #876

merged 2 commits into from
Apr 5, 2017

Conversation

mtho11
Copy link
Contributor

@mtho11 mtho11 commented Apr 1, 2017 •
edited
Loading

This adds intelligent defaults when Adding JDBC Driver instead of having to know what the options are to key in.

jdbc-defaults mov

Here is the old(current) Add JDBC Driver screen:
add-driver-original
We really can't do much here as far as validations because the above scenario is correct -- so there needs to be very loose validation. To counter this, we will provide intelligent defaults instead. This eliminates the need to look up these values and is consistent with the Add Datasource screens as they use the same services.

The new screen with intelligent defaults:
manageiq__middleware_servers

This also brings it in alignment with the Add Datasource wizard.

Links

@miq-bot
Copy link
Member

miq-bot commented Apr 1, 2017

Checked commits https://github.com/mtho11/manageiq-ui-classic/compare/80b93afe964e935e1db393ca67d69cd7eab73173~...a6ca0d2ff29860262f0a21988fe6fcd49d858130 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍪

@mtho11
Copy link
Contributor Author

mtho11 commented Apr 2, 2017 •
edited
Loading

@miq-bot add_label middleware, enhancement

@miq-bot
Copy link
Member

miq-bot commented Apr 2, 2017

@mtho11 unrecognized command 'middleware', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

@mtho11
Copy link
Contributor Author

mtho11 commented Apr 2, 2017

@miq-bot add_label middleware, enhancement

@mtho11
Copy link
Contributor Author

mtho11 commented Apr 2, 2017

@miq-bot assign @abonas

@abonas
Copy link
Member

abonas commented Apr 2, 2017

very nice. where are the defaults taken from?
In the future, please keep the formatting changes (which are very welcome!) in a separate PR as the formatting is not related to the functionality change and it makes it hard to review, and requires stepping in and out separate commits here.

abonas
abonas reviewed Apr 2, 2017
View reviewed changes
app/assets/javascripts/controllers/middleware_server/middleware_server_controller.js Outdated
@@ -121,11 +122,32 @@ function MwServerControllerFactory($scope, miqService, isGroupDeployment) {
$scope.resetJdbcDriverForm = function () {
$scope.jdbcDriverModel = {};
$scope.jdbcDriverModel.serverId = angular.element('#server_id').val();
$scope.jdbcDriverModel.xaDatasource = true;
Copy link
Member

Choose a reason for hiding this comment

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

why is XA default = true?

app/assets/javascripts/controllers/middleware_server/middleware_server_controller.js
@@ -195,7 +217,7 @@ function ServerGroupOpsService($http, $q) {

function ServerOpsServiceFactory($http, $q, isGroup) {
var runOperation = function runOperation(id, operation, timeout) {
var errorMsg = isGroup ? _('Error running operation on this server.')
var errorMsg = isGroup ? _('Error running operation on this server.')
Copy link
Member

Choose a reason for hiding this comment

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

is this just a formatting change? if so, it should not be part of this commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, sorry about that.

app/views/middleware_server/_add_jdbc_driver.html.haml
%label.col-md-4.control-label{:for => "choose_datasource_input"}
= _("Datasource:")
.col-md-8
%select.form-control#chooose_driver_ds{"name" => "choose_driver_ds_input",
Copy link
Member

Choose a reason for hiding this comment

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

this looks like a comment - is this a github issue? (the choose driver part above)

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, that is a github issue as the #choose_driver_ds is the actual id

app/assets/javascripts/controllers/middleware_server/middleware_server_controller.js
function MwServerController($scope, miqService) {
return MwServerControllerFactory($scope, miqService, false);
function MwServerController($scope, miqService, mwAddDatasourceService) {
return MwServerControllerFactory($scope, miqService, mwAddDatasourceService, false);
Copy link
Member

Choose a reason for hiding this comment

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

what is the false (and the true in line 36) parameter standing for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comes from function MwServerControllerFactory($scope, miqService, mwAddDatasourceService, isGroupDeployment) so it is just a flag to tell the factory whether we are dealing with Servers or ServerGroups while maintaining the same Factory signature.

@mtho11 mtho11 mentioned this pull request Apr 2, 2017
Merged
@mtho11
Copy link
Contributor Author

mtho11 commented Apr 2, 2017

@abonas defaults were taken from WildFly/EAP 7.

@mtho11
Copy link
Contributor Author

mtho11 commented Apr 2, 2017

@abonas totally agree with the formatting changes separate.

@abonas
Copy link
Member

abonas commented Apr 3, 2017

@abonas defaults were taken from WildFly/EAP 7.

what I meant is where are they now kept in miq?

@mtho11
Copy link
Contributor Author

mtho11 commented Apr 4, 2017

@abonas The static defaults data is here: https://github.com/mtho11/manageiq-ui-classic/blob/bc3d90bd4d27295595da5d01fdb28b8f940a610d/app/assets/javascripts/controllers/middleware_datasource/middleware_datasource_service.js#L9-L9

@abonas
Copy link
Member

abonas commented Apr 5, 2017

@abonas The static defaults data is here: https://github.com/mtho11/manageiq-ui-classic/blob/bc3d90bd4d27295595da5d01fdb28b8f940a610d/app/assets/javascripts/controllers/middleware_datasource/middleware_datasource_service.js#L9-L9

thanks. although not part of this PR, I am wondering - why is a service under the controllers folder and not in the services folder?

@abonas
Copy link
Member

abonas commented Apr 5, 2017

LGTM
@miq-bot assign @martinpovolny

@miq-bot miq-bot assigned martinpovolny and unassigned abonas Apr 5, 2017
@martinpovolny martinpovolny merged commit a63ff3c into ManageIQ:master Apr 5, 2017
@martinpovolny
Copy link
Member

@mtho11 , @abonas : fine?

@martinpovolny martinpovolny added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 5, 2017
@abonas
Copy link
Member

abonas commented Apr 5, 2017

@miq-bot add_label fine/yes

simaishi pushed a commit that referenced this pull request Apr 5, 2017
@martinpovolny @simaishi
Merge pull request #876 from mtho11/jdbc_driver_defaults
98a75d6 
Add Smart Jdbc driver defaults
(cherry picked from commit a63ff3c)
@simaishi
Copy link
Contributor

simaishi commented Apr 5, 2017

Fine backport details:

$ git log -1
commit 98a75d6fb1114f9c133a9952ef88c5a1003dad5c
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Wed Apr 5 19:46:21 2017 +0200

    Merge pull request #876 from mtho11/jdbc_driver_defaults
    
    Add Smart Jdbc driver defaults
    (cherry picked from commit a63ff3cda19bfc7de00f8251e0f53a485cd9dbd5)

@mtho11 mtho11 deleted the jdbc_driver_defaults branch April 7, 2017 14:15
@mtho11
Copy link
Contributor Author

mtho11 commented Apr 7, 2017

@miq-bot add_label angular dialogs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abonas abonas abonas left review comments

Assignees

@martinpovolny martinpovolny

Labels
angular dialogs enhancement fine/backported middleware
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mtho11 @miq-bot @abonas @martinpovolny @simaishi