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.

Sign up for GitHub

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

Jump to bottom

Azure Databases for MySQL Server #855

Merged
merged 17 commits into from
Apr 3, 2020
Merged

Azure Databases for MySQL Server #855

merged 17 commits into from
Apr 3, 2020

Conversation

frodopwns
Copy link
Contributor

@frodopwns frodopwns commented Mar 31, 2020 •
edited by jananivMS
Loading

Closes #875

What this PR does / why we need it:
MySQL Server, Database, FirewallRule

Special notes for your reviewer:
test it similarly to Azure SQL or PSQL

@frodopwns frodopwns changed the title WIP: Azure Databases for MySQL Server Azure Databases for MySQL Server Apr 1, 2020
@buhongw7583c buhongw7583c self-requested a review April 2, 2020 06:31
@jananivMS jananivMS self-requested a review April 2, 2020 17:21
jananivMS
jananivMS previously requested changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@jananivMS jananivMS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good overall. Tested the scenarios and works well. There were a couple typos from copy-paste and a couple other suggestions

pkg/resourcemanager/mysql/database/client.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/database/reconcile.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/database/reconcile.go Show resolved Hide resolved
pkg/resourcemanager/mysql/database/reconcile.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/firewallrule/reconcile.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/server/server_test.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/server/server_test.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/server/server_test.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/server/server_test.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/server/suite_test.go Outdated Show resolved Hide resolved
@frodopwns frodopwns requested a review from jananivMS April 3, 2020 02:38
@frodopwns frodopwns dismissed jananivMS’s stale review April 3, 2020 02:39

changes addressed

buhongw7583c
buhongw7583c reviewed Apr 3, 2020
View reviewed changes
Copy link
Contributor

@buhongw7583c buhongw7583c left a comment

Choose a reason for hiding this comment

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

Cannot verify this on China Azure though as there is no Quota.

config/samples/azure_v1alpha1_mysqldatabase.yaml Outdated Show resolved Hide resolved
config/samples/azure_v1alpha1_mysqlfirewallrule.yaml Outdated Show resolved Hide resolved
config/samples/azure_v1alpha1_mysqlserver.yaml Outdated Show resolved Hide resolved
@jananivMS
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jananivMS
jananivMS previously approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@jananivMS jananivMS left a comment

Choose a reason for hiding this comment

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

lgtm. Works well for the testing I did and tests also pass locally

frodopwns and others added 2 commits April 3, 2020 13:52
@frodopwns
Merge branch 'master' into mysql
763b78b
@frodopwns @jananivMS
Apply suggestions from code review
c2083fb 
Co-Authored-By: Janani Vasudevan <49576785+jananivMS@users.noreply.github.com>
@jananivMS jananivMS self-requested a review April 3, 2020 21:43
@jananivMS
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jananivMS
jananivMS approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@jananivMS jananivMS left a comment

Choose a reason for hiding this comment

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

lgtm. Tests passed again locally

@frodopwns frodopwns merged commit 94ee6d7 into Azure:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buhongw7583c buhongw7583c buhongw7583c left review comments

@jananivMS jananivMS jananivMS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Task: Create an operator and CRD for MySQL service
3 participants
@frodopwns @jananivMS @buhongw7583c