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

[FEEL FREE TO FINISH] New module: proxysql_cluster #27

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

akimrx
Copy link
Contributor

@akimrx akimrx commented May 9, 2021

SUMMARY

IF ANYONE WANTS TO FINISH IT BECAUSE OF #26 (comment), FEEL FREE TO DO IT!

Added new module proxysql_cluster, which implements server management in the ProxySQL cluster.
Read more here.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

proxysql_cluster

ADDITIONAL INFORMATION

The linked module proxysql_manage_config has also been updated.

@codecov
Copy link

codecov bot commented May 9, 2021

Codecov Report

Merging #27 (6dc9442) into main (beb3841) will increase coverage by 0.10%.
The diff coverage is 71.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
+ Coverage   70.02%   70.12%   +0.10%     
==========================================
  Files           8       10       +2     
  Lines         994     1329     +335     
  Branches      164      220      +56     
==========================================
+ Hits          696      932     +236     
- Misses        222      295      +73     
- Partials       76      102      +26     
Impacted Files Coverage Δ
plugins/modules/proxysql_manage_config.py 64.00% <ø> (ø)
plugins/modules/proxysql_cluster.py 71.06% <71.06%> (ø)
...ugins/modules/proxysql_query_rules_fast_routing.py 69.88% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beb3841...6dc9442. Read the comment docs.

@Andersson007
Copy link
Contributor

@akimrx thanks for the new module!
Could you please revert renaming of the ignore.txt

@akimrx
Copy link
Contributor Author

akimrx commented May 10, 2021

Hi! Yep, a little later

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@akimrx thanks for the module!

Most the comments from the other PR is relevant here.
When it's ready for review, please put it explicitly and I'll ping the other folks to review.

Thanks.

plugins/modules/proxysql_cluster.py Outdated Show resolved Hide resolved
plugins/modules/proxysql_cluster.py Outdated Show resolved Hide resolved
extends_documentation_fragment:
- community.proxysql.proxysql.managing_config
- community.proxysql.proxysql.connectivity

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

you could also put the seealso section, with a link to related proxysql documentation. E.g. from postgresq_pg_hba module

114 seealso:
115 - name: PostgreSQL pg_hba.conf file reference
116   description: Complete reference of the PostgreSQL pg_hba.conf file documentation.
117   link: https://www.postgresql.org/docs/current/auth-pg-hba-conf.html

Just an idea, not necessary

plugins/modules/proxysql_cluster.py Outdated Show resolved Hide resolved
plugins/modules/proxysql_cluster.py Outdated Show resolved Hide resolved
plugins/modules/proxysql_cluster.py Outdated Show resolved Hide resolved
cursor.execute(query_string, query_data)
return True

def update_server_config(self, cursor):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def update_server_config(self, cursor):
def update_server_config(self, cursor):

can we pass it only once when initializing the object and use self.cursor everywhere?

plugins/modules/proxysql_cluster.py Show resolved Hide resolved
plugins/modules/proxysql_cluster.py Outdated Show resolved Hide resolved
plugins/modules/proxysql_cluster.py Outdated Show resolved Hide resolved
@Andersson007 Andersson007 added the help wanted Extra attention is needed label Jul 19, 2021
@Andersson007 Andersson007 changed the title New module: proxysql_cluster [FEEL FREE TO FINISH] New module: proxysql_cluster Jul 19, 2021
@Andersson007
Copy link
Contributor

If a specialist reviews, tests, and provides feedback that the c.proxysql users will get what they expect, we could merge the module

@markuman if you could take a look, it would be really great

short_description: Adds or removes proxysql cluster hosts using the B(ProxySQL) admin interface
description:
- The M(community.proxysql.proxysql_cluster) module adds or removes
ProxySQL cluster hosts using the B(ProxySQL) admin interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ProxySQL cluster hosts using the B(ProxySQL) admin interface.
ProxySQL cluster hosts using the B(ProxySQL) admin interface.
version_added: '1.2.0'

@Andersson007
Copy link
Contributor

FYI: Whoever finishes this, i believe it would be fair if they put themselves in the authors: list

@markuman
Copy link
Member

markuman commented Aug 9, 2021

If a specialist reviews, tests, and provides feedback that the c.proxysql users will get what they expect, we could merge the module

@markuman if you could take a look, it would be really great

I can take over. But I'd like to address #60, #61 and #62 first and release 1.2.0
So this will land in 1.3.0 if no one else will do it before :)

@Andersson007
Copy link
Contributor

If a specialist reviews, tests, and provides feedback that the c.proxysql users will get what they expect, we could merge the module
@markuman if you could take a look, it would be really great

I can take over. But I'd like to address #60, #61 and #62 first and release 1.2.0
So this will land in 1.3.0 if no one else will do it before :)

sounds very good to me

@markuman
Copy link
Member

If a specialist reviews, tests, and provides feedback that the c.proxysql users will get what they expect, we could merge the module
@markuman if you could take a look, it would be really great

I can take over. But I'd like to address #60, #61 and #62 first and release 1.2.0
So this will land in 1.3.0 if no one else will do it before :)

I will target #19 for the next release.
the benefits of using proxysql in cluster mode are - imo - marginal. Most of the benefits are already targeted by using an orchestration tool like ansible.
If someone has some time to complete this, feel free to adopt this pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants