-
Notifications
You must be signed in to change notification settings - Fork 260
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
[activemq_xml] adding integration #521
Conversation
[activemq_xml] fixing tests + flakes
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.
Looks good but docs need to be updated.
let(:facts) {{ | ||
operatingsystem: 'Ubuntu', | ||
}} | ||
if enabled |
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.
if is_agent_5
would be way more readable, since it's what really means. Why are we calling this enabled
?
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.
I see it's written like this in every integration rspec, so it's probably better to keep it consistent unless we change it everywhere.
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.
Updated it everywhere 👍
<% if !instance['supress_errors'].nil? -%> | ||
supress_errors: <%= instance['supress_errors'] %> | ||
<%- end -%> | ||
<%- if !instance['detailed_queues'].nil? and unless instance['detailed_queues'].empty? -%> |
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.
Mixing unless
and if !
. Would be more idiomatic to always use unless
.
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.
You're right this isn't very idiomatic - and it's just rooted at my subpar ruby. I just always struggle with the readability of compound logical statements with unless
because you never know if the other side of the or/and statement is also getting negated, and feel it ends up being less readable. I'll change it regardless.
# The username for the datadog user | ||
# $ssl | ||
# Boolean to enable SSL | ||
# $use_psycopg2 |
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.
docs copy-pasted from postgres integration :) and not updated.
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.
Added one comment but LGTM.
# | ||
# Parameters: | ||
# $url | ||
# URL to gather the activemq_xml starts from |
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.
Maybe mention this should point to ActiveMQ web admin's URL, and not to the service itself.
* [activemq_xml] adding integration [activemq_xml] fixing tests + flakes * [activemq_xml] fixing documentation * [activemq_xml][spec] improve readability * [spec] improve agent5 test switch readability * [activemq_xml] better docs
No description provided.