-
Notifications
You must be signed in to change notification settings - Fork 63
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
Volume-related graph refresh fixes & VCR specs #74
Conversation
ed8a27e
to
24998ac
Compare
Kicking Travis as master is now green |
This pull request is not mergeable. Please rebase and repush. |
@zeari @enoodle @moolitayer Please review. |
I'm all for setting out base test cluster for infra stuff + template - that's what we deploy mostly anyway. |
:builder_params => {:ems_id => manager.id}, | ||
:association => :persistent_volume_claims, | ||
:use_ar_object => true, # serialized :capacity attr | ||
# TODO: add container_project_id column to persistent_volume_claims. |
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.
Can you please make this an issue? an issue item is also 👍
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.
=> #78. Removing TODO.
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 👍
@zakiva can you review? |
This pull request is not mergeable. Please rebase and repush. |
For consistency with get_custom_attributes_graph + better error.
Testing those in the new easier-to-reproduce VCRs.
if volume.persistentVolumeClaim.try(:claimName) | ||
new_result[:persistent_volume_claim_ref] = { | ||
:namespace => pod.metadata.namespace, | ||
:name => volume.persistentVolumeClaim.try(:claimName), |
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.
nit, but I think you can omit the try
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.
done
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
Checked commits cben/manageiq-providers-kubernetes@a376f3c~...17e6f9e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@cben I thought the test label is for patches that are test but I'm not sure. wdyt? |
Added refresher specs for CVs, PVs, PVCs.
Testing those in the new easier-to-reproduce VCRs. I was lazy and avoided re-recording but relying on some hawkular/cassandra stuff that's not in the template — but should be pretty stable.
@miq-bot add-label bug, test
Graph refresh RFE: https://bugzilla.redhat.com/show_bug.cgi?id=1470021