Notification after Tower credential CUD operations #14625

jameswnl · 2017-04-03T23:54:17Z

For https://bugzilla.redhat.com/show_bug.cgi?id=1437628

@miq-bot add_labels wip, bug, providers/ansible_tower

bdunne · 2017-04-04T20:50:22Z

...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb

+        refresh(manager)
+        record = find_by!(:manager_id => manager.id, :manager_ref => project.id)
+      rescue AnsibleTowerClient::ClientError => tower_error
+        _log.error "create_in_provider failed: #{tower_error}"


The create_in_provider in all of these message should not be necessary because _log already prefixes with the class, separator, and method.

Also, you don't need to log this since ansible_tower_client will already log its failures to this log file.

got it. so we don't need logging for the RecordNotFound as well, right?

bdunne · 2017-04-04T20:52:06Z

...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb

-      project = manager.with_provider_connection do |connection|
-        connection.api.projects.create!(params)
-      end
+      begin


I don't think you need the begin / end wrapper because you can do:

def create_in_provider(manager_id, params) stuff rescue FirstError error stuff rescue SecondError error stuff end

bdunne · 2017-04-04T21:09:46Z

...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb

+        record = find_by!(:manager_id => manager.id, :manager_ref => project.id)
+      rescue AnsibleTowerClient::ClientError => tower_error
+        _log.error "create_in_provider failed: #{tower_error}"
+        error = tower_error


I'm not a fan of this. I'd rather see:

def create_in_provider(manager_id, params) manager = ExtManagementSystem.find(manager_id) project = manager.with_provider_connection do |connection| connection.api.projects.create!(params) end refresh(manager) find_by!(:manager_id => manager.id, :manager_ref => project.id).tap do notify('create_in_provider', manager.id, params, true) end rescue AnsibleTowerClient::ClientError => tower_error notify('create_in_provider', manager.id, params, false) raise rescue ActiveRecord::RecordNotFound => not_found_error _log.error "Failed to find record: #{not_found_error.message}" notify('create_in_provider', manager.id, params, false) raise end

Actually, me neither. See my new patch. thanks

bdunne · 2017-04-04T21:10:54Z

...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb

+      _log.error "update_in_provider failed: #{tower_error}"
+      error = tower_error
+    rescue ActiveRecord::RecordNotFound => not_found_error
+      _log.error "update_in_provider couldn't locate record: #{not_found_error}"


But this is an instance method... how would you hit this?

I was thinking the case that the Tower object got deleted and refresh destroyed this.

But I guess chances are low. I've removed it.

bdunne · 2017-04-04T21:11:38Z

...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb

+      end
+      self.class.send('refresh', manager)
+      reload
+    rescue AnsibleTowerClient::ClientError => tower_error


This pattern could probably be extracted to a method.

yeah, the python decorator would be nice. Does ruby has something similar?

bdunne · 2017-04-04T21:12:37Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/credential.rb

@@ -28,6 +38,17 @@ def create_in_provider_queue(manager_id, params)

    private

+    def notify(op, manager_id, params, success)


This is identical to the method in the other file...

I have #14453 in the making.

Do you have other suggestion as to where to move this?

bdunne · 2017-04-04T21:13:49Z

spec/support/ansible_shared/automation_manager/configuration_script_source.rb

      expect(AnsibleTowerClient::Connection).to receive(:new).and_return(atc)
      store_new_project(project, manager)
      expect(EmsRefresh).to receive(:queue_refresh_task).and_return([finished_task])
      expect(ExtManagementSystem).to receive(:find).with(manager.id).and_return(manager)
-
+      expect(projects).to receive(:create!).with(params)
+      expect(Notification).to receive(:create).with(expected_notify).and_return(double(Notification))


Do you need to return anything? I don't see it being used anywhere.

True, no need. Removed. thanks

bdunne · 2017-04-04T21:14:45Z

spec/support/ansible_shared/automation_manager/configuration_script_source.rb

      expect { described_class.create_in_provider(manager.id, params) }.to raise_error(ActiveRecord::RecordNotFound)
    end

-    it ".create_in_provider_queue" do
+    it "#create_in_provider_queue" do


This is a class method, please leave the ., not #

got it. updated

miq-bot · 2017-04-05T08:37:36Z

This pull request is not mergeable. Please rebase and repush.

jameswnl · 2017-04-05T16:19:31Z

@miq-bot remove_label wip

jameswnl · 2017-04-06T14:28:32Z

@miq-bot add_label fine/yes

bdunne · 2017-04-06T19:34:39Z

...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb

+  rescue AnsibleTowerClient::ClientError => error
+    raise
+  ensure
+    self.class.send('notify', 'update_in_provider', manager.id, params, error.nil?)


Why send here?

notify is a private class_method and this is inside an instance method

bdunne · 2017-04-06T19:35:15Z

...ls/manageiq/providers/ansible_tower/shared/automation_manager/configuration_script_source.rb

+  rescue AnsibleTowerClient::ClientError => error
+    raise
+  ensure
+    self.class.send('notify', 'delete_in_provider', manager.id, "manager_ref=#{manager_ref}", error.nil?)


Why send here?

bdunne · 2017-04-06T19:35:37Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/credential.rb

+  rescue AnsibleTowerClient::ClientError => error
+    raise
+  ensure
+    self.class.send('notify', 'update_in_provider', resource.id, params, error.nil?)


Why send here?

bdunne · 2017-04-06T19:35:45Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/credential.rb

+  rescue AnsibleTowerClient::ClientError => error
+    raise
+  ensure
+    self.class.send('notify', 'delete_in_provider', resource.id, "manager_ref=#{manager_ref}", error.nil?)


Why send here?

bdunne · 2017-04-06T19:50:37Z

spec/support/ansible_shared/automation_manager/credential.rb

      expect { described_class.create_in_provider(manager.id, params) }.to raise_error(ActiveRecord::RecordNotFound)
    end

-    it ".create_in_provider_queue" do
+    it "#create_in_provider_queue" do


This is a class method, it should remain as ., not #.

bdunne · 2017-04-06T19:50:55Z

spec/support/ansible_shared/automation_manager/credential.rb

      expect(described_class.create_in_provider(manager.id, params)).to be_a(described_class)
    end

-    it "not found during refresh" do
+    it "#create_in_provider to fail (not found during refresh) and send notification" do


., not #

missed... done now

bdunne · 2017-04-06T19:51:38Z

spec/support/ansible_shared/automation_manager/credential.rb


-    it "#delete_in_provider" do
+    it ".delete_in_provider to succeed and send notification" do


This is an instance method, so #, not .

bdunne · 2017-04-06T19:51:46Z

spec/support/ansible_shared/automation_manager/credential.rb

      ansible_cred.delete_in_provider
    end

-    it "#delete_in_provider_queue" do
+    it ".delete_in_provider to fail (finding credential) and send notification" do


#, not .

no wonder.... i remember i did them, did them at wrong places 😱

miq-bot · 2017-04-06T21:12:34Z

This pull request is not mergeable. Please rebase and repush.

bdunne · 2017-04-07T18:57:16Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/tower_api.rb

+      if defined?(API_ATTRIBUTES) && params.kind_of?(Hash)
+        params.each do |k, _v|
+          if self::API_ATTRIBUTES[k] && self::API_ATTRIBUTES[k][:type] == :password
+            params[k] = '******'


You probably don't want to modify the original, right?

jameswnl · 2017-04-07T21:00:59Z

@bdunne see my new patch.

jameswnl · 2017-04-10T15:57:55Z

@simaishi backporting of this to Fine depends on backporting #14453

bdunne · 2017-04-10T16:14:14Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/tower_api.rb

      queue(manager.my_zone, nil, "create_in_provider", [manager_id, params], action)
    end

    private

+    def notify(op, manager_id, params, success)
+      notify_params = params


I was about to comment on unnecessary assignment until I read further. Can you improve the readability here? I think the code in the block is complicated enough to justify its own method.

I've pulled that out to be its own method in its class.

bdunne

Just a comment about readability. Otherwise looks fine

jameswnl · 2017-04-07T19:09:02Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/tower_api.rb

      queue(manager.my_zone, nil, "create_in_provider", [manager_id, params], action)
    end

    private

+    def notify(op, manager_id, params, success)
+      if defined?(API_ATTRIBUTES) && params.kind_of?(Hash)


@bdunne @blomquisg I've added this since last review to prevent secrets being shown in notification.

bdunne · 2017-04-10T17:57:29Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/tower_api.rb

      queue(manager.my_zone, nil, "create_in_provider", [manager_id, params], action)
    end

    private
+    def notify(op, manager_id, params, success)
+      params = hide_secrets(params) if respond_to? :hide_secrets


Normally we require parenthesis around method arguments. Unfortunately it's hard for Rubocop to catch, so they don't look for it. http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Lint/RequireParentheses

Ok, I've added them.

can because of ManageIQ#14685

miq-bot · 2017-04-10T18:48:13Z

Checked commits jameswnl/manageiq@0d7c2e7~...7359776 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 🍰

jameswnl · 2017-04-11T01:30:42Z

@bdunne Thanks!

Notification after Tower credential CUD operations (cherry picked from commit 4f7363a)

simaishi · 2017-04-11T15:29:31Z

Fine backport details:

$ git log -1
commit 3bdf50f0498153f073466d2ce68c478558c88c4f
Author: Brandon Dunne <[email protected]>
Date:   Mon Apr 10 21:29:29 2017 -0400

    Merge pull request #14625 from jameswnl/notification
    
    Notification after Tower credential CUD operations
    (cherry picked from commit 4f7363a9d5fc590f5c7208e888c0fc1056545bb7)

miq-bot added bug providers/ansible_tower wip labels Apr 3, 2017

jameswnl force-pushed the notification branch 2 times, most recently from 5d03a30 to 84b0f71 Compare April 4, 2017 17:20

chessbyte assigned blomquisg Apr 4, 2017

jameswnl force-pushed the notification branch from 373ef0c to 5728b9c Compare April 4, 2017 20:33

bdunne requested changes Apr 4, 2017

View reviewed changes

miq-bot added the unmergeable label Apr 5, 2017

jameswnl changed the title ~~[WIP] Notification after Tower credential CUD operations~~ Notification after Tower credential CUD operations Apr 5, 2017

jameswnl force-pushed the notification branch from 187e649 to cf44afd Compare April 5, 2017 16:20

miq-bot removed the wip label Apr 5, 2017

miq-bot added the fine/yes label Apr 6, 2017

jameswnl force-pushed the notification branch from cf44afd to f5f5080 Compare April 6, 2017 18:16

miq-bot removed the unmergeable label Apr 6, 2017

jameswnl force-pushed the notification branch from f5f5080 to 5b7a0fd Compare April 6, 2017 18:31

bdunne requested changes Apr 6, 2017

View reviewed changes

jameswnl force-pushed the notification branch from 5b7a0fd to 49ab02a Compare April 6, 2017 20:17

miq-bot added the unmergeable label Apr 6, 2017

jameswnl force-pushed the notification branch from 49ab02a to 0c8ea82 Compare April 6, 2017 22:45

miq-bot removed the unmergeable label Apr 6, 2017

jameswnl force-pushed the notification branch 3 times, most recently from baa2aec to 00ca670 Compare April 7, 2017 19:02

bdunne requested changes Apr 7, 2017

View reviewed changes

bdunne reviewed Apr 10, 2017

View reviewed changes

jameswnl commented Apr 10, 2017

View reviewed changes

bdunne reviewed Apr 10, 2017

View reviewed changes

jameswnl added 7 commits April 10, 2017 14:40

Tower project/credential CUD to send notification

0d7c2e7

Spec tests for Tower credential/project CUD notificaiton

bb5f05f

don't send secrets in Tower CUD notification

ea5673b

Tower CUD cares only input of symbolic key

ced7fac

can because of ManageIQ#14685

Notification to use a new hash

93eae87

hide_secrets before notification of Tower credential operations

29a1f7a

added parenthesis for calls to respond_to?

7359776

jameswnl force-pushed the notification branch from 04fbff7 to 7359776 Compare April 10, 2017 18:41

bdunne approved these changes Apr 10, 2017

View reviewed changes

bdunne merged commit 4f7363a into ManageIQ:master Apr 11, 2017

bdunne added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 11, 2017

jameswnl deleted the notification branch April 11, 2017 01:30

simaishi pushed a commit that referenced this pull request Apr 11, 2017

Merge pull request #14625 from jameswnl/notification

3bdf50f

Notification after Tower credential CUD operations (cherry picked from commit 4f7363a)

simaishi added fine/backported and removed fine/yes labels Apr 11, 2017

ZitaNemeckova mentioned this pull request Sep 20, 2017

Change error notification level from success to error #15998

Merged

Fryguy added the providers/automation label Jan 24, 2020

Fryguy unassigned blomquisg Jan 24, 2020

		@@ -28,6 +38,17 @@ def create_in_provider_queue(manager_id, params)

		private

		def notify(op, manager_id, params, success)


		it "#delete_in_provider" do
		it ".delete_in_provider to succeed and send notification" do

Notification after Tower credential CUD operations #14625

Notification after Tower credential CUD operations #14625

Conversation

jameswnl commented Apr 3, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Apr 5, 2017

jameswnl commented Apr 5, 2017

jameswnl commented Apr 6, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Apr 6, 2017

Choose a reason for hiding this comment

jameswnl commented Apr 7, 2017

jameswnl commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne left a 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

miq-bot commented Apr 10, 2017

jameswnl commented Apr 11, 2017

simaishi commented Apr 11, 2017