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

Update default proxysql version, drop support for xenial (EOL) and fix small bugs #92

Merged
merged 8 commits into from
Feb 23, 2022

Conversation

mrpk1906
Copy link
Contributor

@mrpk1906 mrpk1906 commented Feb 17, 2022

Hi all,

This PR contain changes:

  • Update default proxysql version to 2.3.2 (latest)
  • Drop support for xenial
  • Remove undefined variables: (removed from 2.0.13) mysql-default_sql_mode, mysql-default_time_zone, mysql-forward_autocommit. Ref: https://proxysql.com/documentation/global-variables
  • Hide sensitive data (username, password) when set_fact
  • Change percona repo url to https

I tested on Ubuntu 18 and Ubuntu 20. It works well

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #92 (e14bee4) into main (c527827) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #92   +/-   ##
=======================================
  Coverage   72.97%   72.97%           
=======================================
  Files          10       10           
  Lines        1199     1199           
  Branches      193      193           
=======================================
  Hits          875      875           
  Misses        246      246           
  Partials       78       78           

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 c527827...e14bee4. Read the comment docs.

@markuman
Copy link
Member

Works also for me on Ubuntu 18.04 and 20.04

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.

Should we say that the role requires Python3 since now? Wouldn't it be a breaking change? (i don't remember the context of the PR)

@@ -21,7 +21,7 @@ jobs:
strategy:
matrix:
proxysql:
- 2.0.12
- 2.3.2
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
- 2.3.2
- 2.3.2

I would suggest having at least the oldest version of proxysql supported by the collection and the latest one there (no need to add now, just an idea)

Copy link
Member

Choose a reason for hiding this comment

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

That's something we can place/add to this issue #75 :)

@markuman
Copy link
Member

markuman commented Feb 23, 2022

Should we say that the role requires Python3 since now? Wouldn't it be a breaking change? (i don't remember the context of the PR)

I think it is a natural thing, because the installation role supports just ubuntu 16.04 and 18.04 until now: https://github.com/ansible-collections/community.proxysql/blob/main/roles/proxysql/meta/main.yml

And after dropping 16.04 (because it is EOL since arpil 2021), there is no need to take care about python2 anymore in Ubuntu 18.04 and 20.04.

@markuman
Copy link
Member

And after dropping 16.04 (because it is EOL since arpil 2021), there is no need to take care about python2 anymore in Ubuntu 18.04 and 20.04.

...and Ubuntu 22.04 is knocking the door.
But that's another Issue, And if the collection role should focus also on other distributions ....

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.

@markuman thanks for clarifying, the PR LGTM

@markuman markuman merged commit 4a7e884 into ansible-collections:main Feb 23, 2022
@Andersson007
Copy link
Contributor

@mrpk1906 thanks for the contribution! @markuman thanks for reviewing and testing!

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.

None yet

3 participants