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

feature support more customization #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

noushi
Copy link

@noushi noushi commented Jan 12, 2019

Hello,

This PR adds support for:

  1. free form proxies (for stats for ex.)
  2. haproxy-configure tag (to speed up reconfigs)

Regards

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@noushi Thanks for your PR. I've made a couple of comments, let me know what you think.

@@ -11,6 +11,8 @@
backup: false
dest: /etc/logrotate.d/haproxy
src: logrotated-haproxy.j2
tags:
- haproxy-configure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why this could be useful, but this is just one of our many openmicroscopy roles and playbooks so I'd prefer not to add tags to individual roles unless we have an overall plan.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, I'll keep it in my own code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, do you know when/if you will have such a plan?
That tag name actually comes from @geerlingguy's original role.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid we don't have a timeline yet.

@@ -36,6 +36,11 @@ defaults
errorfile 504 /etc/haproxy/errors/504.http
{% endif %}

{% for proxy in haproxy_proxies %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no structure to this section, i.e. there's nothing here that makes this specific to proxies, it could be used for any arbitrary configuration. How about move it to the end of the file, and call it something like haproxy_additional_config?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe the role has to allow for any configurations. This makes the role usable outside the scope of the currently intended/supported uses.

I would like to add support for the stats entry later on.

I agree for the name and location. I will update this.

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