Skip to content
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

Merged
merged 5 commits into from
May 14, 2019
Merged

[activemq_xml] adding integration #521

merged 5 commits into from
May 14, 2019

Conversation

truthbk
Copy link
Member

@truthbk truthbk commented May 10, 2019

No description provided.

[activemq_xml] fixing tests + flakes
Copy link
Contributor

@albertvaka albertvaka left a 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
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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? -%>
Copy link
Contributor

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.

Copy link
Member Author

@truthbk truthbk May 14, 2019

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
Copy link
Contributor

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.

Copy link
Contributor

@albertvaka albertvaka left a 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
Copy link
Contributor

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.

@truthbk truthbk merged commit cc69348 into master May 14, 2019
@truthbk truthbk deleted the jaime/activemq branch May 14, 2019 18:17
@truthbk truthbk added this to the 2.6.0 milestone May 31, 2019
cegeka-jenkins pushed a commit to cegeka/puppet-datadog_agent that referenced this pull request Apr 6, 2020
* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants