Skip to content

Add MySQLCluster v1beta2#350

Merged
zoetrope merged 29 commits intomainfrom
d-kuro/conversion
Jan 18, 2022
Merged

Add MySQLCluster v1beta2#350
zoetrope merged 29 commits intomainfrom
d-kuro/conversion

Conversation

@d-kuro
Copy link
Copy Markdown
Contributor

@d-kuro d-kuro commented Nov 24, 2021 •
edited
Loading

refs: #337

Add MySQLCluster v1beta2 API and conversion webhook.

MySQLCluster v1beta2

Split the .spec.serviceTemplate into .spec.primaryServiceTemplate and .spec.replicaServiceTemplate.

Befor:

spec:
  serviceTemplate:
    metadata:
      labels:
        foo: bar

After:

spec:
  primaryServiceTemplate:
    metadata:
      labels:
        foo: bar
  replicaServiceTemplate:
    metadata:
      labels:
        foo: baz

@d-kuro d-kuro self-assigned this Nov 24, 2021
@d-kuro d-kuro force-pushed the d-kuro/conversion branch 11 times, most recently from 2c14690 to 822d697 Compare December 1, 2021 04:07
@d-kuro d-kuro force-pushed the d-kuro/conversion branch 5 times, most recently from 87c021a to ef6babf Compare December 7, 2021 12:25
d-kuro
d-kuro commented Dec 8, 2021
View reviewed changes
d-kuro
d-kuro commented Dec 8, 2021
View reviewed changes
d-kuro
d-kuro commented Dec 8, 2021
View reviewed changes
d-kuro
d-kuro commented Dec 8, 2021
View reviewed changes
d-kuro
d-kuro commented Dec 8, 2021
View reviewed changes
d-kuro
d-kuro commented Dec 8, 2021
View reviewed changes
d-kuro
d-kuro commented Dec 8, 2021
View reviewed changes
charts/moco/CHANGELOG.md

## [Unreleased]

### Support MySQLCluster v1beta2 API
Copy link
Copy Markdown
Contributor Author

@d-kuro d-kuro Dec 8, 2021

Choose a reason for hiding this comment

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

Manual operations are required to support conversion webhooks in Helm Chart, which is documented in the changelog.

@d-kuro d-kuro changed the title WIP: Add MySQLCluster v1beta2 Add MySQLCluster v1beta2 Dec 8, 2021
@d-kuro d-kuro marked this pull request as ready for review December 8, 2021 00:49
ymmt2005
ymmt2005 suggested changes Dec 8, 2021
View reviewed changes
@d-kuro @ymmt2005
Fix Makefile format.
6e11466 
Co-authored-by: Yamamoto, Hirotaka <ymmt2005@gmail.com>
@d-kuro d-kuro requested a review from ymmt2005 December 28, 2021 19:10
ymmt2005
ymmt2005 suggested changes Jan 3, 2022
View reviewed changes
controllers/mysqlcluster_controller.go Outdated
Comment on lines +874 to +878
var bucketConfig mocov1beta2.BucketConfig

if err := mocov1beta1.Convert__BucketConfig_To_v1beta2_BucketConfig(&jc.BucketConfig, &bucketConfig, nil); err != nil {
return fmt.Errorf("failed to convert bucket config from v1beta1 to v1beta2: %w", err)
}
Copy link
Copy Markdown
Member

@ymmt2005 ymmt2005 Jan 3, 2022

Choose a reason for hiding this comment

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

We should have retrieved BackupPolicy with v1beta2 at line 841 so that
this conversion can be removed.
4f5c0ae#diff-67f34c9b4cabb4950b383eab55b6ebeeebc524fa70c888bdc132662c16a7899fR841

Copy link
Copy Markdown
Contributor Author

@d-kuro d-kuro Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your review comments!

I assumed that I would only create the v1beta2 API for MySQLCluster and continue to use v1beta1 without upgrading the BackupPolicy.
This conversion is necessary because MySQLCluster (v1beta2 package) and BackupPolicy (v1beta1 package) use a common JobConfig structure.

Or should I create a v1beta2 API for BackupPolicy too, even if there are no changes to the API?

Copy link
Copy Markdown
Member

@ymmt2005 ymmt2005 Jan 4, 2022

Choose a reason for hiding this comment

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

@d-kuro

Or should I create a v1beta2 API for BackupPolicy too, even if there are no changes to the API?

I think so.

@zoetrope Thoughts?

Copy link
Copy Markdown
Member

@zoetrope zoetrope Jan 5, 2022

Choose a reason for hiding this comment

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

I think the same version would be better.

Copy link
Copy Markdown
Contributor Author

@d-kuro d-kuro Jan 12, 2022

Choose a reason for hiding this comment

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

I added v1beta2.BackupPolicy

  • Add BackupPolicy v1beta2: d0fa6af
  • Use BackupPolicy v1beta2: 7c02683
  • Add conversion webhook for BackupPolicy: 4cc80be

d-kuro added 3 commits January 8, 2022 21:28
@d-kuro
Add BackupPolicy v1beta2
d0fa6af 
Signed-off-by: d-kuro <kurosawa7620@gmail.com>
@d-kuro
Use BackupPolicy v1beta2
7c02683 
Signed-off-by: d-kuro <kurosawa7620@gmail.com>
@d-kuro
Add conversion webhook for BackupPolicy
4cc80be 
Signed-off-by: d-kuro <kurosawa7620@gmail.com>
@d-kuro d-kuro force-pushed the d-kuro/conversion branch from 802b183 to 4cc80be Compare January 10, 2022 11:37
@d-kuro d-kuro requested a review from ymmt2005 January 12, 2022 04:08
ymmt2005
ymmt2005 reviewed Jan 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ymmt2005 ymmt2005 left a comment

Choose a reason for hiding this comment

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

@d-kuro
It looks almost good to me. Thank you.

Please update apiVersion in examples and documents.
I believe this is the last one.

@zoetrope
Copy link
Copy Markdown
Member

zoetrope commented Jan 13, 2022

@d-kuro
Reviewed. And I confirmed that conversion webhook works.
It's fine.

I have one question, why did you change test package from v1beta1 to v1beta1_test?

d-kuro added 3 commits January 16, 2022 21:04
@d-kuro
Add BackupPolicy v1beta2 docs
e2400d9 
Signed-off-by: d-kuro <kurosawa7620@gmail.com>
@d-kuro
Remove old api docs
3a55e6b 
Signed-off-by: d-kuro <kurosawa7620@gmail.com>
@d-kuro
Fix testing package name
05700b9 
Signed-off-by: d-kuro <kurosawa7620@gmail.com>
@d-kuro
Copy link
Copy Markdown
Contributor Author

d-kuro commented Jan 16, 2022

@ymmt2005

Please update apiVersion in examples and documents.
I believe this is the last one.

Thanks! I added BackupPolicy v1beta2 docs.
e2400d9

@d-kuro
Copy link
Copy Markdown
Contributor Author

d-kuro commented Jan 16, 2022

@zoetrope

I have one question, why did you change test package from v1beta1 to v1beta1_test?

Thank you comments.
I changed the package to avoid circular imports, but I realized it wasn't necessary now.
Fixed: 05700b9

@d-kuro d-kuro requested a review from ymmt2005 January 16, 2022 17:01
@ymmt2005
Copy link
Copy Markdown
Member

ymmt2005 commented Jan 16, 2022 •
edited
Loading

@d-kuro Please update apiVersion in examples and documents.
For example, https://github.com/cybozu-go/moco/blob/d-kuro/conversion/examples/anti-affinity.yaml#L2

$ git grep moco.cybozu.com/v1beta1 docs examples
docs/custom-mysqld.md:apiVersion: moco.cybozu.com/v1beta1
docs/troubles.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
docs/usage.md:apiVersion: moco.cybozu.com/v1beta1
examples/anti-affinity.yaml:apiVersion: moco.cybozu.com/v1beta1
examples/collect-metrics.yaml:apiVersion: moco.cybozu.com/v1beta1
examples/custom-mycnf.yaml:apiVersion: moco.cybozu.com/v1beta1
examples/guaranteed.yaml:apiVersion: moco.cybozu.com/v1beta1
examples/loadbalancer.yaml:apiVersion: moco.cybozu.com/v1beta1

Also, please fix the usage of serviceTemplate in the documents.

$ git grep serviceTemplate docs examples
docs/reconcile.md:The Services' labels, annotations, and `spec` fields can be customized with MySQLCluster's `status.serviceTemplate` field.
docs/usage.md:  serviceTemplate:
examples/loadbalancer.yaml:  # serviceTemplate allows you to specify annotations, labels, and spec
examples/loadbalancer.yaml:  serviceTemplate:

ymmt2005
ymmt2005 suggested changes Jan 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ymmt2005 ymmt2005 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.

Documents and examples haven't been updated yet.

d-kuro added 2 commits January 17, 2022 02:55
@d-kuro
Upgrade apiVersion for example and docs.
fcbc2a5 
Signed-off-by: d-kuro <kurosawa7620@gmail.com>
@d-kuro
Update serviceTemplate usage docs.
6625785 
Signed-off-by: d-kuro <kurosawa7620@gmail.com>
@d-kuro
Copy link
Copy Markdown
Contributor Author

d-kuro commented Jan 16, 2022

@ymmt2005
Sorry, I must have missed that.
I fixed it:
fcbc2a5
6625785

@d-kuro d-kuro requested a review from ymmt2005 January 16, 2022 18:09
ymmt2005
ymmt2005 reviewed Jan 17, 2022
View reviewed changes
ymmt2005
ymmt2005 approved these changes Jan 17, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ymmt2005 ymmt2005 left a comment

Choose a reason for hiding this comment

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

LGTM

zoetrope
zoetrope approved these changes Jan 18, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@zoetrope zoetrope left a comment

Choose a reason for hiding this comment

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

LGTM

@zoetrope zoetrope merged commit 13e9421 into main Jan 18, 2022
@zoetrope zoetrope deleted the d-kuro/conversion branch January 18, 2022 01:17
@arosh arosh mentioned this pull request Jan 28, 2022
Closed
@zoetrope zoetrope mentioned this pull request Feb 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zoetrope zoetrope zoetrope approved these changes

@masa213f masa213f Awaiting requested review from masa213f

+1 more reviewer

@ymmt2005 ymmt2005 ymmt2005 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@d-kuro d-kuro

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@d-kuro @zoetrope @ymmt2005