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

Adding PostgreSQL 13 and MySQL 8.3 to GitHub Actions. #345

Merged
merged 8 commits into from
Aug 29, 2024

Conversation

hiroyuki-sato
Copy link
Member

  • Adding PostgreSQL 13 and MySQL 8.3 to GitHub Actions.
  • Change JDK image temurin to zulu.

@hiroyuki-sato hiroyuki-sato requested a review from a team as a code owner August 22, 2024 08:26
Copy link
Member

@dmikurube dmikurube left a comment

Choose a reason for hiding this comment

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

Left some comments and suggestions.

.github/workflows/check.yml Outdated Show resolved Hide resolved
.github/workflows/check.yml Outdated Show resolved Hide resolved
run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "SELECT user, host, plugin FROM mysql.user;"
- name: Change password mechanism1 (root@localhost)
run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH mysql_native_password BY 'root';"
- name: Change password mechanism2 (root@%)
Copy link
Member

@dmikurube dmikurube Aug 27, 2024

Choose a reason for hiding this comment

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

What are the password mechanism1 and the password mechanism2 ? Can you leave a link to a kind of official documents?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's remove mechanism1 and mechanism2.
The first part(mechanism1) is for root@localhost
The second part(mechanism2) is for root@%

  • Change password mechanism1 (root@localhost) -> Change password (root@localhost)
  • Change password mechanism2 (root@%) -> Change password (root@%)

Copy link
Member

Choose a reason for hiding this comment

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

For account names like ...@localhost and ...@%, you may want to leave a link to: https://dev.mysql.com/doc/refman/8.4/en/account-names.html

Copy link
Member Author

Choose a reason for hiding this comment

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

.github/workflows/check.yml Outdated Show resolved Hide resolved
Comment on lines 94 to 96
#
# End caching_sha2_password workaround.
#
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

It meant the mark of the end of the workaround.

# This part change password mechanism to mysql_native_password.
# Remove the following part after update Connector/J
#
- name: Show password plugins
run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "SELECT user, host, plugin FROM mysql.user;"
- name: Change password mechanism1 (root@localhost)
run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH mysql_native_password BY 'root';"
- name: Change password mechanism2 (root@%)
run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "ALTER USER 'root'@'%' IDENTIFIED WITH mysql_native_password BY 'root';"
- name: FLUSH PRIVILEGES
run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "FLUSH PRIVILEGES;"
- name: Show password plugins2
run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "SELECT user, host, plugin FROM mysql.user;"
#
# End caching_sha2_password workaround.
#

Copy link
Member

Choose a reason for hiding this comment

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

Rather than such a beginning mark and an end mark, I'd suggest to leave individual comments for each step.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed beginning mark and an end mark

.github/workflows/check.yml Outdated Show resolved Hide resolved
@hiroyuki-sato
Copy link
Member Author

@dmikurube Thank you for your review. I'll fix it later.

@hiroyuki-sato
Copy link
Member Author

@dmikurube Thank you for your review. Please retake a look when you have time.

Copy link
Member

@dmikurube dmikurube left a comment

Choose a reason for hiding this comment

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

It basically looks good, then approved, but left a couple of additional comments. Please go ahead with addressing those comments.

run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH mysql_native_password BY 'root';"
- name: Change password (root@%)
run: mysql -h 127.0.0.1 --port 3306 -uroot -proot -e "ALTER USER 'root'@'%' IDENTIFIED WITH mysql_native_password BY 'root';"
- name: FLUSH PRIVILEGES
Copy link
Member

Choose a reason for hiding this comment

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

Is this FLUSH PRIVILEGES required to make the password (mechanism) changes effective? If then, can you explain it in the comment?

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 misunderstood about flush privileges. It was not require. So, I remove it.

.github/workflows/check.yml Outdated Show resolved Hide resolved
@hiroyuki-sato
Copy link
Member Author

@dmikurube Thank you for your review. I fixed your comments I'll merge them later.

@hiroyuki-sato hiroyuki-sato merged commit 99d8603 into master Aug 29, 2024
12 checks passed
@hiroyuki-sato hiroyuki-sato deleted the embulk/add-mysql8-and-postgrsql13 branch August 29, 2024 11:26
@hiroyuki-sato
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants