Add a concern for storing and accessing embedded ansible object ids #14377
Conversation
| default_ansible_objects.find_by(:name => name).try(:value).try(:to_i) | ||
| end | ||
|
|
||
| def set_default_ansible_object(name, value) |
There was a problem hiding this comment.
Will the value be tower ref id?
There was a problem hiding this comment.
It will be the tower side database id, this PR allows it to be any value, but we're setting it to the id in the other PR here https://github.com/ManageIQ/manageiq/pull/14283/files#diff-45e99ec23caedc019723f05f7817652eR22
| raise DefaultAnsibleObjectError, "#{name} object already exists with value #{current_value}" | ||
| end | ||
|
|
||
| default_ansible_objects.create(:name => name, :value => value) |
There was a problem hiding this comment.
All of this feels like default_ansible_objects.find_or_initialize_by(:name => name).update_attributes(:value => value). This will be a noop if the record already exists or have the same existing value. Not sure why you need the check on lines 58-60
There was a problem hiding this comment.
I specifically wanted to disallow changing the value once it was created. I feel like that could lead to some really weird issues.
Do you think that's necessary?
| expect { subject.public_send("default_#{obj_name}=", obj_name.length + 1) }.to raise_error(described_class::DefaultAnsibleObjectError) | ||
| end | ||
|
|
||
| it "#default_#{obj_name}= raises an error if we try to set to a new value" do |
There was a problem hiding this comment.
Looks like you have a duplicate test here.
There was a problem hiding this comment.
Ah thanks I think I meant this to test that it worked if we tried to set the value to the same value ... Good catch!
| end | ||
|
|
||
| context "with attributes saved" do | ||
| before do |
There was a problem hiding this comment.
I think you can use a before(:context) here to create all four records up front rather than creating four records before each test (16 times)
There was a problem hiding this comment.
That doesn't work because subject is per example.
There was a problem hiding this comment.
Not a fan of before(:context) for the same reason we don't do before(:all) or before(:suite). You're just begging for test contamination. Incidentally before(:context) is just a synonym for before(:all)
| module ManageIQ::Providers::EmbeddedAnsible::Provider::DefaultAnsibleObjects | ||
| extend ActiveSupport::Concern | ||
|
|
||
| class DefaultAnsibleObjectError < RuntimeError; end |
There was a problem hiding this comment.
I think this should this live with the rest of our exceptions, not in this class.
|
This pull request is not mergeable. Please rebase and repush. |
carbonin
force-pushed
the
add_custom_attrs_for_default_ansible_objs
branch
from
a74ccc6
to
2cdc2a5
Compare
…ct ids This will allow us to determine the ansible side object to relate new objects to. This also frees us from having to store an extra flag in our inventory to identify which objects are the "system created" ones and which are the other objects that may be artifacts of running jobs. https://www.pivotaltracker.com/story/show/140098929
carbonin
force-pushed
the
add_custom_attrs_for_default_ansible_objs
branch
from
2cdc2a5
to
5921219
Compare
| @@ -2,4 +2,44 @@ | |||
|
|
|||
| describe ManageIQ::Providers::EmbeddedAnsible::Provider do | |||
| it_behaves_like 'ansible provider' | |||
|
|
|||
| subject { FactoryGirl.build(:provider_embedded_ansible) } | |||
There was a problem hiding this comment.
From #14283 (comment)
@bdunne
Why not FactoryGirl.create rather than adding the subject.save in the before block?
@carbonin
Ah, I think this was a rebase issue. I started this branch before #14383 was merged so instead of changing the subject for all the tests, I saved the record in just my context, during the rebase I think this line got left here.
I'll refactor in a new commit so that the individual specs define the subject rather than the shared example.
Thanks for catching this.
This ensures that the specific provider under test implements the defined behavior rather than just running the same set of specs twice (what was being done with the shared subject). Intorduced in eb6be29
|
Checked commits carbonin/manageiq@5921219~...06a52e2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
This will allow us to determine the ansible side object to relate new objects to.
This also frees us from having to store an extra flag in our inventory to identify which objects are the "system created" ones and which are the other objects that may be artifacts of running jobs.
https://www.pivotaltracker.com/story/show/140098929