-
Notifications
You must be signed in to change notification settings - Fork 52
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.
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
Add Container Template Spec #37
Conversation
@miq-bot add_label test |
fd48ada
to
b5c483e
Compare
:port => "443"}, | ||
:authentication => {:role => :hawkular, | ||
:auth_key => token, | ||
:userid => "_"}}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we spell out these provider connection_configurations all over the place.
can you check if they are actually the same, and make a factory that includes them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually have a factory for this provider. I think it might be more convenient this way in case you need to rerecord the changes (and change the hostname, password etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
template = FactoryGirl.create(:container_template, | ||
:ems_id => ems.id, | ||
:objects => [object], | ||
:object_labels => object_labels).tap do |temp| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zakiva travis is complaining about this line:
undefined method
object_labels=' for #ContainerTemplate:0xXXX`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this PR depends on ManageIQ/manageiq#15406, ManageIQ/manageiq#15587 (it's also mentioned in the PR description)
ManageIQ/manageiq-providers-kubernetes#81 was merged |
ahh that won't fix anything here. well ¯_(ツ)_/¯ |
I believe all dependent PRs are merged. |
94122d3
to
e47a4e4
Compare
123d598
to
330ead5
Compare
Checked commit zakiva@330ead5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@moolitayer Added the factory here, PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Adding test for template instantiation with object labels and parameters.
Depends on ManageIQ/manageiq#15406, ManageIQ/manageiq#15587