Collect variable Tower data upon population #92

Glutexo · 2018-05-29T10:52:51Z

Each time objects are populated in a Tower instance, some of their properties (ids, timestamps…) change. The same applies to objects counts that can differ from the previous state. This information is used then in the specs.

Collect these changing data during their population in Tower instance to a YAML file. This allows to fill the necessary information in the specs after re-recording the cassettes more easily. Eventually it could be used directly from the tests too.

Extracted from #79 for easier review. This part contains only changes to the Rake task. Goes along with #93.

@miq-bot add_reviewer @jameswnl

miq-bot · 2018-06-04T18:35:59Z

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

Glutexo · 2018-06-05T11:20:01Z

#89 got merged. Rebased this to resolve conflicts. Reused the get_obj method introduced by the previous PR and utilized the waiting to collect the data after they become available.

bronaghs · 2018-06-19T16:06:26Z

@Glutexo - Can you resolve the conflicts so we can get this in

Glutexo · 2018-06-21T07:11:20Z

Rebased on current master, conflicts resolved.

miq-bot · 2018-06-21T09:20:01Z

Some comments on commit Glutexo@4acd332

lib/tasks_private/spec_helper.rake

⚠️ - 293 - Detected puts. Remove all debugging statements.
⚠️ - 341 - Detected puts. Remove all debugging statements.
⚠️ - 345 - Detected puts. Remove all debugging statements.

miq-bot · 2018-06-21T09:20:13Z

Checked commit Glutexo@4acd332 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 5 offenses detected

lib/tasks_private/spec_helper.rake

❗ - Line 341, Col 11 - Style/FormatStringToken - Prefer template tokens (like %{foo}) over unannotated tokens (like %s).
❗ - Line 341, Col 14 - Style/FormatStringToken - Prefer template tokens (like %{foo}) over unannotated tokens (like %s).
❗ - Line 341, Col 18 - Style/FormatStringToken - Prefer template tokens (like %{foo}) over unannotated tokens (like %s).
❗ - Line 345, Col 17 - Style/FormatStringToken - Prefer template tokens (like %{foo}) over unannotated tokens (like %s).
❗ - Line 345, Col 21 - Style/FormatStringToken - Prefer template tokens (like %{foo}) over unannotated tokens (like %s).

tumido · 2018-07-19T13:59:44Z

@jameswnl, I see here that miq-bot has some suggestions which are being resolved in the #82 PR. Can this one be reviewed and merged. I'll resolve the eventual conflicts in the #82 .

jameswnl · 2018-07-19T21:00:08Z

lib/tasks_private/spec_helper.rake

    del_obj(uri, data['name'])
    obj = JSON.parse(@conn.post(uri, data).body)
    puts "%s %s %s" % ["Created name=#{obj['name']}".ljust(40), "manager_ref/ems_ref= #{obj['id']}".ljust(30), "url=#{obj['url']}".ljust(10)]
+
+    dumped_data = obj.slice(*dumped_fields)
+    @tower_data.update { |td| td['items'][obj['name']] = dumped_data } unless dumped_data.empty?


Can you move the @tower_data.update part out of this method? Let it just does the creation of object in Tower.

jameswnl · 2018-07-19T21:12:07Z

lib/tasks_private/spec_helper.rake

+
+  def update
+    yield @data if block_given?
+    write


this write each time something updated seems too much. yeah, it's not a big deal in this rake task. but ....

I agree that this excessive writing just as the whole TowerData class might be an overkill. I’d just like to explain my motivation to that:

Because the population of a Tower instance can easily end with some failure during the process, I wanted to have the YAML file always consistent with what has been actually created in Tower. It’s always possible to revert it and it can serve as a reference.

But as @tumido pointed out, it is not actually necessary, because the YAML is intended to be used solely by Specs. And it can be used only if complete. For reference you can always read the terminal output or actually look into the Tower web UI.

jameswnl · 2018-07-19T21:28:28Z

lib/tasks_private/spec_helper.rake

+  end
+end
+
+class TowerData


This is pretty cool. However, it's overkill in this case and that introduces unnecessary mental weight for forthcoming maintainers. Can we simply dump the collected hash into yml after creating the data set?

jameswnl · 2018-07-19T21:35:16Z

spec/support/tower_data_helper.rb

@@ -0,0 +1,21 @@
+module Spec


Similarly here, I think we don't need this to achieve what this PR intends to. In #93 you can simply load from either a given yml file or the default one.

miq-bot · 2018-07-20T14:34:36Z

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

Each time objects are populated in a Tower instance, some of their properties (ids, timestamps…) change. The same applies to objects counts that can differ from the previous state. This information is used then in the specs. Collect these changing data during their population in Tower instance to a YAML file. This allows to fill the necessary information in the specs after re-recording the cassettes more easily. Eventually it could be used directly from the tests too.

tumido · 2018-07-23T17:20:13Z

@jameswnl I think it is acceptable now. Can you please review it? Thanks 👍 🍷

jameswnl

Hey Tom, almost there

jameswnl · 2018-07-23T18:10:18Z

lib/tasks_private/spec_helper.rake

  end

  def v3_2?
    @version >= Gem::Version.new('3.2')
  end

+  def save


This object is PopulateTower and so a method named as save would be ambiguous as to where it's saving to. How about rename it explicitly as to_file, data_to_file or dump?

jameswnl · 2018-07-23T18:16:23Z

lib/tasks_private/spec_helper.rake

-    data = JSON.parse(@conn.get(uri).body)
-    data['results'].each do |item|
+    obj = get_obj(uri)
+    obj['results'].each do |item|
      next if item['name'] != match_name
      puts "Deleting old #{item['name']}: #{item['url']}"
      @conn.delete(item['url'])


Found a bug not related to your changes. We should exit this method here by return @conn.delete(item['url']) since we found our target. Otherwise, Line 112 will still be executed and is simply doing useless work.

I've refactored the method so it may be a bit cleaner and better to understand what it does. Tell me if you like it 😉 If not, I can revert it.

jameswnl · 2018-07-23T18:29:42Z

lib/tasks_private/spec_helper.rake

  #
  require 'faraday'
  require 'faraday_middleware'

  MAX_TRIES = 10
  TRY_SLEEP = 2
  DEL_SLEEP = 20
+  TOWER_DATA_YAML = ManageIQ::Providers::AnsibleTower::Engine.root.join('spec', 'support', 'tower_data.yml')


Remove this line and pass the file path as parameter when dumping to file (Line 11). This way just from reading the calling code of this class, people would know what to expect.

And you don't have to split the path into 3 parts, you can simply jam them into 1 literal string being appended to the engine root.

Sure! About the 3 parts.. I just copy-pasted that without thinking, thanks for pointing that out. 🚀

jameswnl · 2018-07-23T21:46:52Z

lib/tasks_private/spec_helper.rake

+
+    populate.create_dataset
+    populate.counts
+    populate.to_file(ManageIQ::Providers::AnsibleTower::Engine.root.join("spec/support/tower_data.yml"))


single ' 😄

Should I open a new PR? 😄

I don’t like the fact that the path is here in the Rake task. Since the YAML is going to be used by the Specs, I’d prefer to have the constant somewhere along them and use it from here.

Glutexo · 2018-07-24T07:52:27Z

Merged! ᔧ! 🎉 Thanks guys for taking care of it. I hope this will actually make the use of the Rake task more pleasant for its future users.

miq-bot requested a review from jameswnl May 29, 2018 10:53

Glutexo force-pushed the collect_populated_tower_data branch from 73786c0 to 9aca802 Compare May 29, 2018 10:59

This was referenced May 29, 2018

Extract variable Tower data from tests #93

Merged

[wip] Collect variable tower data #79

Closed

Capture object references when populating test data #78

Closed

miq-bot added the unmergeable label Jun 4, 2018

Glutexo force-pushed the collect_populated_tower_data branch 2 times, most recently from e81a0d3 to f32ebcf Compare June 5, 2018 11:17

Glutexo force-pushed the collect_populated_tower_data branch from f32ebcf to d7a22ad Compare June 21, 2018 07:10

Glutexo force-pushed the collect_populated_tower_data branch from d7a22ad to 4acd332 Compare June 21, 2018 09:14

Glutexo mentioned this pull request Jun 21, 2018

Populate project without update job in Tower #82

Merged

1 task

bronaghs assigned jameswnl Jun 21, 2018

miq-bot removed the unmergeable label Jul 3, 2018

jameswnl reviewed Jul 19, 2018

View reviewed changes

jameswnl suggested changes Jul 19, 2018

View reviewed changes

miq-bot added the unmergeable label Jul 20, 2018

Glutexo and others added 2 commits July 20, 2018 19:46

Adjust count method puts

7cde8eb

tumido force-pushed the collect_populated_tower_data branch 3 times, most recently from 5b7555c to 09a2540 Compare July 23, 2018 16:13

jameswnl reviewed Jul 23, 2018

View reviewed changes

Remove reduntand classes and cleanup exported YAML

8b807dc

tumido force-pushed the collect_populated_tower_data branch from 09a2540 to 8b807dc Compare July 23, 2018 21:03

jameswnl reviewed Jul 23, 2018

View reviewed changes

jameswnl approved these changes Jul 23, 2018

View reviewed changes

jameswnl merged commit 205e4b5 into ManageIQ:master Jul 23, 2018

miq-bot removed the unmergeable label Jul 24, 2018

Glutexo deleted the collect_populated_tower_data branch July 24, 2018 07:52

bronaghs added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect variable Tower data upon population #92

Collect variable Tower data upon population #92

Glutexo commented May 29, 2018 •

edited

Loading

miq-bot commented Jun 4, 2018

Glutexo commented Jun 5, 2018

bronaghs commented Jun 19, 2018

Glutexo commented Jun 21, 2018

miq-bot commented Jun 21, 2018

miq-bot commented Jun 21, 2018

tumido commented Jul 19, 2018

jameswnl Jul 19, 2018

jameswnl Jul 19, 2018

Glutexo Jul 24, 2018

jameswnl Jul 19, 2018

jameswnl Jul 19, 2018

miq-bot commented Jul 20, 2018

tumido commented Jul 23, 2018

jameswnl left a comment

jameswnl Jul 23, 2018

jameswnl Jul 23, 2018

tumido Jul 23, 2018

jameswnl Jul 23, 2018

tumido Jul 23, 2018

jameswnl Jul 23, 2018

tumido Jul 23, 2018

Glutexo Jul 24, 2018

Glutexo commented Jul 24, 2018 •

edited

Loading

Collect variable Tower data upon population #92

Collect variable Tower data upon population #92

Conversation

Glutexo commented May 29, 2018 • edited Loading

miq-bot commented Jun 4, 2018

Glutexo commented Jun 5, 2018

bronaghs commented Jun 19, 2018

Glutexo commented Jun 21, 2018

miq-bot commented Jun 21, 2018

miq-bot commented Jun 21, 2018

tumido commented Jul 19, 2018

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 Jul 20, 2018

tumido commented Jul 23, 2018

jameswnl 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

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

Glutexo commented Jul 24, 2018 • edited Loading

Glutexo commented May 29, 2018 •

edited

Loading

Glutexo commented Jul 24, 2018 •

edited

Loading