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

Support capi v1alpha2 #140

Merged
merged 8 commits into from
Mar 18, 2020
Merged

Support capi v1alpha2 #140

merged 8 commits into from
Mar 18, 2020

Conversation

prksu
Copy link
Contributor

@prksu prksu commented Jan 26, 2020

What this PR does / why we need it:

This PR update cluster-api-provider-digitalocean to be support with v1alpha2 cluster-api types

Which issue(s) this PR fixes
Fixes #139

Special notes for your reviewer:
/cc xmudrii

I separate the commit to providing a better way to review since this PR has large changes.

Documentation:

Release note:

Support Cluster API v1alpha2

@k8s-ci-robot k8s-ci-robot requested a review from xmudrii January 26, 2020 19:21
@k8s-ci-robot
Copy link

Welcome @prksu!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-digitalocean 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-digitalocean has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 26, 2020
@k8s-ci-robot
Copy link

Hi @prksu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 26, 2020
@prksu
Copy link
Contributor Author

prksu commented Jan 26, 2020

/hold

before merging this PR, we should create a new branch for current workv1alpha1 #ref

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2020
@prksu prksu mentioned this pull request Jan 26, 2020
Closed
@prksu
Copy link
Contributor Author

prksu commented Jan 26, 2020

friendly ping @nikhita

@nikhita
Copy link
Member

nikhita commented Jan 27, 2020

/assign @xmudrii

@prksu prksu mentioned this pull request Jan 27, 2020
Merged
@prksu prksu force-pushed the support-capi-v1alpha2 branch from bd7de62 to f35a8f2 Compare January 30, 2020 13:34
prksu
prksu commented Jan 30, 2020
View reviewed changes
Copy link
Contributor Author

@prksu prksu left a comment

Choose a reason for hiding this comment

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

This commit reinitializes this project using kubebuilder. most of the file in this commit are generated from kubebuilder.

@xmudrii
Copy link
Member

xmudrii commented Jan 30, 2020

@prksu Thanks for the PR! I'll try to review it by the end of the week.

@prksu
Copy link
Contributor Author

prksu commented Feb 3, 2020

Great! please let me know any your suggestion about this approach @xmudrii

@xmudrii xmudrii mentioned this pull request Feb 12, 2020
Closed
cpanato
cpanato reviewed Feb 12, 2020
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
@cpanato
Copy link
Member

cpanato commented Feb 12, 2020

huge pr, did some initial checking, will look more tomorrow

@prksu prksu force-pushed the support-capi-v1alpha2 branch from f35a8f2 to 072ab36 Compare February 13, 2020 12:01
@prksu
Copy link
Contributor Author

prksu commented Feb 13, 2020

@cpanato thanks for checking this PR, any suggestions are welcome

@xmudrii
Copy link
Member

xmudrii commented Mar 16, 2020

I've created a PR to update jobs, so tests can pass and the PR can get merged. kubernetes/test-infra#16802

detiber
detiber reviewed Mar 16, 2020
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

Overall this looks like some pretty impressive work, given all the changes involved.

Added a few comments based on a quick pass through the PR.

controllers/suite_test.go Outdated Show resolved Hide resolved
controllers/suite_test.go Outdated Show resolved Hide resolved
controllers/suite_test.go Outdated Show resolved Hide resolved
controllers/domachine_controller.go Outdated Show resolved Hide resolved
controllers/domachine_controller.go Outdated Show resolved Hide resolved
controllers/domachine_controller.go Outdated Show resolved Hide resolved
controllers/domachine_controller.go Outdated Show resolved Hide resolved
controllers/domachine_controller.go Outdated Show resolved Hide resolved
controllers/docluster_controller.go Outdated Show resolved Hide resolved
controllers/docluster_controller.go Outdated Show resolved Hide resolved
@xmudrii
Copy link
Member

xmudrii commented Mar 16, 2020

/test all

@prksu prksu force-pushed the support-capi-v1alpha2 branch from dd809d4 to b07a3c4 Compare March 17, 2020 04:57
@prksu prksu force-pushed the support-capi-v1alpha2 branch from b07a3c4 to 0022db7 Compare March 17, 2020 05:16
@prksu prksu force-pushed the support-capi-v1alpha2 branch from 0022db7 to 357700d Compare March 17, 2020 10:44
@xmudrii
Copy link
Member

xmudrii commented Mar 17, 2020

@prksu Do you have any idea why tests fail? Can we fix this by the time we merge the PR?

cpanato
cpanato reviewed Mar 17, 2020
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
cpanato
cpanato reviewed Mar 17, 2020
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
@prksu
Copy link
Contributor Author

prksu commented Mar 17, 2020

@xmudrii it seems because sigs.k8s.io/controller-runtime/pkg/envtest requires kubebuilder installed on the host.

other providers have scripts directory that uses for related ci jobs. maybe we need to have too, and let make target as the current state but we need to update the prow jobs config again, wdyt?

@prksu prksu mentioned this pull request Mar 17, 2020
Closed
6 tasks
This was referenced Mar 17, 2020
Merged
Merged
@prksu prksu force-pushed the support-capi-v1alpha2 branch from 357700d to 15f1418 Compare March 18, 2020 02:47
@xmudrii
Copy link
Member

xmudrii commented Mar 18, 2020

/test all

@xmudrii
Copy link
Member

xmudrii commented Mar 18, 2020

@prksu I think the PR looks amazing! 💯 🎉 Thank you so much for all the hard work and all the time you've spent migrating it to v1alpha2! I'll go ahead and merge the PR. If anything additional is needed, we can do it in a follow-up.

I'll see about getting the bucket for images later today.

@cpanato @detiber Thank you so much for helping me to review the PR!

:shipit:

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 18, 2020
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: prksu, xmudrii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 86fb87f into kubernetes-sigs:master Mar 18, 2020
@prksu prksu mentioned this pull request Mar 19, 2020
Closed
4 tasks
@prksu prksu deleted the support-capi-v1alpha2 branch March 29, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@detiber detiber detiber left review comments

@cpanato cpanato cpanato left review comments

@xmudrii xmudrii xmudrii left review comments

Assignees

@detiber detiber

@xmudrii xmudrii

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support cluster-api v1alpha2
6 participants
@prksu @k8s-ci-robot @nikhita @xmudrii @cpanato @detiber