-
Notifications
You must be signed in to change notification settings - Fork 898
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
[FINE] Adaptations to have a working refresh for Hawkular's Inventory.v3 #14927
[FINE] Adaptations to have a working refresh for Hawkular's Inventory.v3 #14927
Conversation
…rics - Messaging types are now retrieved from its "type path" because the resources do not anymore include its resource-types. - When collecting availability metrics, the "paths" are unescaped rather than escaped for string comparisons. This is because the paths received from hawkular are percent-encoded with arbitrary case and string comparisons were failing because some paths are using uppercase while others are using lowercase (e.g. '%2f' rather than '%2F').
Disabling expectations testing the underlying container because they require a more complex setup to work correctly. Will be re-enabled in a follow-up commit. Cassettes were recorded using the domain-mode example configuration for WildFly 10.1.0.Final docker image. The cassettes of the event catcher were also modified to work correctly with hawkular ruby client 3.0.1 when the bump is made.
Checked commits israel-hdez/manageiq@1b3735e~...1368321 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
LGTM |
@israel-hdez please take a look at Travis failure. |
@simaishi well.. as expected, the errors are on the cassettes:
As i said in ManageIQ/manageiq-gems-pending#138 (comment), those errors won't go away until the other PRs are backported. |
@israel-hdez Ah, thanks. I remember seeing comment about VCR somewhere 😄 @abonas are you good with this PR? |
end | ||
|
||
def test_port | ||
80 | ||
# 80 |
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.
why is the comment needed?
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.
It is just a reminder that hservices.torii.gva.redhat.com is on port 80. I can remove the comment if you want.
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.
it's a nit, so it's up to you, it doesn't influence the code
@@ -30,15 +30,16 @@ | |||
|
|||
# check whether the server was associated with the vm | |||
server = @ems_hawkular.middleware_servers.first | |||
expect(server.lives_on_id).to eql(@vm.id) | |||
expect(server.lives_on_type).to eql(@vm.type) | |||
# TODO: Restore these expectations |
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.
why is that commented out in this patch?
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.
The service used for these ones is not available now. So, I couldn't record the cassettes against it. A little discussion is in the other PR: ManageIQ/manageiq-providers-hawkular#10 (comment)
should there be a change also in middleware_deployment.rb ? |
@abonas Yes, the equivalent change is here: https://github.com/ManageIQ/manageiq/pull/14927/files#diff-4acd9f4e59821ea6f157f0b901ecc670R183 |
LGTM |
This should be https://bugzilla.redhat.com/show_bug.cgi?id=1446329 then (I guess) |
This is the same work as ManageIQ/manageiq-providers-hawkular#10 but tailored for fine branch.
If ManageIQ/manageiq-providers-hawkular#10 cannot be backported due to conflicts, please merge this PR.
@miq-bot add_label providers/hawkular
\cc @abonas @pilhuhn