CRUD for embedded Ansible credentials #641

mzazrivec · 2017-03-09T16:36:39Z

Implemented:

delete (done in async manner via queue worker)
edit (not fully working b/c of issue #14459)
create (pending pr #14217)

The actual Credential edit / create form is implemented as an angular component.

Pending backend PRs:
ManageIQ/manageiq#14217
ManageIQ/manageiq#14483

Outstanding issues:
ManageIQ/manageiq#14459

AparnaKarve · 2017-03-24T19:24:26Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+      .then(getCredentialOptions)
+      .catch(miqService.handleFailure);
+
+    if (credentialId != 'new') {


should be if (credentialId !== 'new') {

AparnaKarve · 2017-03-24T19:25:11Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+         .catch(miqService.handleFailure);
+    }
+    else {
+      vm.select_options.push({'label':__('<Choose>'), 'value': ''})


semicolon missing

AparnaKarve · 2017-03-24T19:26:05Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+  };
+
+  vm.cancelClicked = function(angularForm) {
+    if (credentialId == 'new') {


same as above, === is recommended.

AparnaKarve · 2017-03-24T19:27:18Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+  function getCredentialOptions(response) {
+    Object.assign(vm.credential_options, response.data.credential_types.embedded_ansible_credential_types);
+
+    for (opt in vm.credential_options) {


Needs to be
for (var opt in vm.credential_options)

(define opt, that is)

AparnaKarve · 2017-03-24T19:27:56Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+      var item = vm.credential_options[vm.credentialModel.type]['attributes'][opt]
+
+      // void the password fields first
+      if (item.hasOwnProperty('type') && item['type'] == 'password') {


AparnaKarve · 2017-03-24T19:30:18Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+    if (warning) {
+      url += '&flash_warning=true&flash_error=false';
+    }
+    else if (error) {


else /else if need to be on the same line as the closing brace for if (Per style guide)
Please fix this here and other places

AparnaKarve · 2017-03-24T21:01:45Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+    // get credential specific options for all supported credential types
+    API.options('/api/authentications')
+      .then(getCredentialOptions)
+      .catch(miqService.handleFailure);


After #783 merge, the .catch will now catch the API failures, but those need to be visible to the user as flash messages.
Created a PR #805 that will display the error message flash.

mzazrivec · 2017-03-24T21:58:05Z

I'm aware of the problem with breadcrumb pointing to invalid url and yes, it needs to be fixed.

But that problem is not specific for nor caused by this PR and will be addressed as a bug
in a different PR.

AparnaKarve · 2017-03-24T23:11:20Z

app/views/ansible_credential/_credential_form.html.haml

+                 'ng-show'       => "vm.afterGet",
+                  "miq-form"     => true,
+                  "model"        => "vm.credentialModel",
+                  "model-copy"   => 'vm.modelCopy',


I recommend using ng-clock here in order to address flickering that is seen sometimes.

ng-cloak => ""
(I just tried it in this form and it seems to have fixed it)

AparnaKarve · 2017-03-24T23:20:30Z

app/assets/javascripts/components/ansible-credential-options.js

+         '</div>',
+         // select
+         '<div ng-switch-when="choice" class="col-md-8">',
+            '<select ng-options="opt as opt for opt in attr.choices" class="form-control pf-select" ng-model="vm.model[name]" />',


pf-select is not quite doing what it's supposed to do.
It's a directive, so try this --

'<div ng-switch-when="choice" class="col-md-8">', '<select pf-select ng-options="opt as opt for opt in attr.choices" class="form-control" ng-model="vm.model[name]" />', '</div>',

AparnaKarve · 2017-03-24T23:22:52Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+        .then(getCredentialFormData)
+        .catch(miqService.handleFailure);
+    }
+    else {


Move else up.

Please do a sweep of the whole JS file for all the else's.

Fixed. And I also did this sweep of the whole JS file .

AparnaKarve · 2017-03-24T23:24:08Z

app/assets/javascripts/controllers/ansible_credentials/ansible_credentials_form_controller.js

+
+    // we need to merge options and vm.credentialModel
+    for (var opt in response.options) {
+      var item = vm.credential_options[vm.credentialModel.type]['attributes'][opt]


another missing semicolon... sorry, didn't catch that before.

AparnaKarve · 2017-03-25T02:09:32Z

Oh, almost forgot...
It would be super if you could add a few jasmine specs...you know, our usual spec requirement prior to a merge.
Thanks.

mzazrivec · 2017-03-27T08:47:08Z

@AparnaKarve I can work on the specs, but if the PR is meant to be merged soon, I'd rather do them in a separate PR.

miq-bot · 2017-03-27T12:13:19Z

Checked commit mzazrivec@e74101c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 11 offenses detected

app/helpers/application_helper/toolbar/ansible_credential_center.rb

❗ - Line 14, Col 26 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 21, Col 92 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 24, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/ansible_credentials_center.rb

❗ - Line 14, Col 34 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 21, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 22, Col 35 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 30, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 31, Col 93 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 34, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

AparnaKarve · 2017-03-27T13:45:23Z

@AparnaKarve I can work on the specs, but if the PR is meant to be merged soon, I'd rather do them in a separate PR.

yeah, works for me.
I believe there is anyway going to be a Part II of this PR soon once the model/API changes are in and once we have something more workable to play with.

AparnaKarve · 2017-03-27T18:10:27Z

@h-kataria I've listed all known issues here:
#641 (comment)
which will be addressed in follow-up PR(s).
Also, there will be a follow-up PR for Jasmine specs.

Unfortunately, due to lack of a working backend, from a reviewer/tester and I suppose even from a developer's point of view, there hasn't really been a "gratification" to see something working completely, in it's entire cycle.
Hopefully, the backend will be resolved soon.
For now, this is GTG.
Thanks.

h-kataria · 2017-03-27T18:33:57Z

@mzazrivec can you remove WIP labels if this is ready for merging

h-kataria · 2017-03-27T20:52:36Z

Removing WIP label from PR and merging, remaining issues and jasmine spec tests will be addressed in follow up PR.

mzazrivec added automation/ansible enhancement labels Mar 9, 2017

mzazrivec changed the title ~~CRUD for embedded Ansible credentials~~ [WIP] CRUD for embedded Ansible credentials Mar 9, 2017

mzazrivec added wip pending core labels Mar 9, 2017

mzazrivec force-pushed the crud_for_embedded_ansible_credentials branch 6 times, most recently from b3d5461 to 15bdb7d Compare March 15, 2017 17:43

mzazrivec force-pushed the crud_for_embedded_ansible_credentials branch 19 times, most recently from fa021e2 to fa3b691 Compare March 23, 2017 18:04

AparnaKarve reviewed Mar 24, 2017

View reviewed changes

mzazrivec force-pushed the crud_for_embedded_ansible_credentials branch from 173deed to 2d4d444 Compare March 24, 2017 21:53

AparnaKarve reviewed Mar 24, 2017

View reviewed changes

mzazrivec force-pushed the crud_for_embedded_ansible_credentials branch from 2d4d444 to e92273f Compare March 27, 2017 08:31

CRUD for embedded Ansible credentials

e74101c

mzazrivec force-pushed the crud_for_embedded_ansible_credentials branch from e92273f to e74101c Compare March 27, 2017 12:10

h-kataria removed the wip label Mar 27, 2017

h-kataria changed the title ~~[WIP] CRUD for embedded Ansible credentials~~ CRUD for embedded Ansible credentials Mar 27, 2017

h-kataria merged commit c17b2f4 into ManageIQ:master Mar 27, 2017

mzazrivec deleted the crud_for_embedded_ansible_credentials branch March 29, 2017 16:44

AparnaKarve mentioned this pull request Mar 30, 2017

Retrieve manager resource from backend and inject in Ansible Credentials form #855

Closed

chessbyte assigned h-kataria Jul 21, 2017

chessbyte added this to the Sprint 57 Ending Mar 27, 2017 milestone Oct 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRUD for embedded Ansible credentials #641

CRUD for embedded Ansible credentials #641

mzazrivec commented Mar 9, 2017 •

edited

Loading

AparnaKarve Mar 24, 2017

mzazrivec Mar 24, 2017

AparnaKarve Mar 24, 2017

mzazrivec Mar 24, 2017

AparnaKarve Mar 24, 2017 •

edited

Loading

mzazrivec Mar 24, 2017

AparnaKarve Mar 24, 2017

mzazrivec Mar 24, 2017

AparnaKarve Mar 24, 2017

mzazrivec Mar 24, 2017

AparnaKarve Mar 24, 2017

mzazrivec Mar 24, 2017

AparnaKarve Mar 24, 2017

mzazrivec commented Mar 24, 2017

AparnaKarve Mar 24, 2017

mzazrivec Mar 27, 2017

AparnaKarve Mar 24, 2017

mzazrivec Mar 27, 2017

AparnaKarve Mar 24, 2017 •

edited

Loading

mzazrivec Mar 27, 2017

AparnaKarve Mar 24, 2017

mzazrivec Mar 27, 2017

AparnaKarve commented Mar 25, 2017

mzazrivec commented Mar 27, 2017

miq-bot commented Mar 27, 2017

AparnaKarve commented Mar 27, 2017

AparnaKarve commented Mar 27, 2017

h-kataria commented Mar 27, 2017

h-kataria commented Mar 27, 2017

CRUD for embedded Ansible credentials #641

CRUD for embedded Ansible credentials #641

Conversation

mzazrivec commented Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzazrivec commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Mar 25, 2017

mzazrivec commented Mar 27, 2017

miq-bot commented Mar 27, 2017

AparnaKarve commented Mar 27, 2017

AparnaKarve commented Mar 27, 2017

h-kataria commented Mar 27, 2017

h-kataria commented Mar 27, 2017

mzazrivec commented Mar 9, 2017 •

edited

Loading

AparnaKarve Mar 24, 2017 •

edited

Loading

AparnaKarve Mar 24, 2017 •

edited

Loading