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

Fixes for ubuntu support #23

Merged
merged 14 commits into from
May 25, 2017
Merged

Fixes for ubuntu support #23

merged 14 commits into from
May 25, 2017

Conversation

mcrauwel
Copy link
Member

@mcrauwel mcrauwel commented Mar 20, 2017

move fixes to seperate PR as request in #19.

@mcrauwel mcrauwel mentioned this pull request Mar 20, 2017
$admin_listen_socket = '/tmp/proxysql_admin.sock'
$package_provider = 'dpkg'
$sys_owner = 'root'
$sys_group = 'root'
}
'Ubuntu': {
$admin_listen_socket = '/tmp/proxysql_admin.sock'
Copy link
Member

Choose a reason for hiding this comment

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

is it a good idea to put a socket to /tmp?

Copy link
Member Author

Choose a reason for hiding this comment

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

/tmp is the upstream ProxySQL default...

Copy link

Choose a reason for hiding this comment

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

Then it's fine by me FWIW.

@@ -2,8 +2,11 @@
prompt = 'Admin> '

[client]
<% if scope.lookupvar('::proxysql::admin_listen_socket') != '' -%>
Copy link
Member

Choose a reason for hiding this comment

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

can we work with undef so this works?

<% if scope.lookupvar('::proxysql::admin_listen_socket') -%>

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been working on this last night but I couldn't get it to work with undef.
The parameter is currently defined as:

String $admin_listen_socket = $::proxysql::params::admin_listen_socket

The default in params is:

$admin_listen_socket = '/tmp/proxysql_admin.sock'

When I change the definition to:

Variant[String, Undef] $admin_listen_socket = $::proxysql::params::admin_listen_socket

Then I can do:

class { '::proxysql': 
 admin_listen_socket => undef,
}

but that will go on and use the default /tmp/proxysql_admin.sock.

Either I leave it like this or I remove the if and the entire else case and we don't support an empty $admin_listen_sock...

Copy link

Choose a reason for hiding this comment

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

I vote empty string is fine then.

@mcrauwel
Copy link
Member Author

@bastelfreak could you try to find some time to finish your review here? thnx ;)

@bastelfreak
Copy link
Member

ah sure!

@bastelfreak bastelfreak merged commit 9d04131 into master May 25, 2017
@bastelfreak bastelfreak deleted the fixes_for_ubuntu_support branch May 25, 2017 13:29
cegeka-jenkins pushed a commit to cegeka/puppet-proxysql that referenced this pull request Aug 22, 2023
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.

4 participants