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

Few modification to puppet-proxysql module #57

Closed
wants to merge 15 commits into from

Conversation

MaxFedotov
Copy link
Contributor

@MaxFedotov MaxFedotov commented Aug 23, 2018

Hello,

I want to introduce a few changes i've added to your module as a PR.

  1. Add ability to automatically create ProxySQL cluster. $cluster_name determines the name of the cluster, ProxySQL instance will belong to. It will lookup host in PuppetDB and create proxysql_cluster resources with params,it will get from it. Also added $cluster_username and $cluster_password variables
  2. Separate config file in two parts - one related to variables (proxysql-main-config-file), another to hostgroups\servers\users\rules configuration (proxysql-proxy-config-file). This was done to allow management of main config file by puppet and manage all proxy configuration settings by some external provider (for example, consul-template, bash script), as this file needs to be updated in more frequent intervals that actual puppet agent interval (e.g. for each failover)
  3. Change the logic on $proxysql::manage_config_file variable. Now config files will be always created by puppet, and $proxysql::manage_main_config_file`$proxysql::manage_proxy_config_file` will define 'replace' setting of file resource
  4. Move directory creation from install to config
  5. Add user and group creation
  6. Change package_source to undef by default
  7. Add $admin_users param. It's array with Linux users, for which we will copy .my.cnf file in their home directory
  8. New param manage_hostgroup_for_servers. This defines which provider will be called from mysql_servers. If true - default (proxy_mysql_server). If false - proxy_mysql_server_no_hostgroup, which don't look on hostgroup field of mysql_server. It is needed in order to allow configuration of this parameters by some external service - consul, script, etc. This can close New versions of proxysql #38
  9. Add ability to configure resources through class parameters using variables ($mysql_servers, $mysql_users, $mysql_hostgroups, $mysql_rules, $schedulers)

@LongLiveCHIEF
Copy link

@MaxFedotov Thank you for contributing! These look great.

Do you think it would be possible to break these up into separate PR's? We like to make PR's in the smallest possible related units, that way we can review and accept code that is ready without holding up features that aren't. It's also useful if we need to revert a PR for some reason. Having a PR for each feature means we can target things better when it comes to maintaining this.

We'd also need to see unit tests in these PR's.

@LongLiveCHIEF
Copy link

lol, you were literally adding the unit tests as I was leaving a comment about this.

@MaxFedotov
Copy link
Contributor Author

Yep :) Sorry for such a lot of commits, I will squash them in one after tests will pass

Maksim Fedotov added 6 commits August 23, 2018 17:03
update docs

fix typo in readme

fix linting for ruby2.4

fix linting

fix for spec tests

fix for spec tests
@LongLiveCHIEF
Copy link

It's not the amount of commits that I'm worried about, it's the number of changes that aren't explicitly dependent upon each other. As far as I can tell, this single PR should really be split into about 4 - 6 PR's.

@MaxFedotov
Copy link
Contributor Author

Ok, i will think about splitting it into more granular parts and make individual PRs for each set of changes

@MaxFedotov
Copy link
Contributor Author

Let's then close this PR and i will make more smaller ones

@mcrauwel
Copy link
Member

I agree with @LongLiveCHIEF

I see a lot of very good work in the large chunk of changes but I also have some questions here and there. Smaller PR's would be a good idea to resolve these issues...

@mcrauwel
Copy link
Member

Per our conversation, closing this huge PR, will be replaced by smaller PR's

@mcrauwel mcrauwel closed this Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New versions of proxysql
3 participants