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

Scaleway create cluster #14641

Merged
merged 5 commits into from
Dec 5, 2022
Merged

Scaleway create cluster #14641

merged 5 commits into from
Dec 5, 2022

Conversation

Mia-Cross
Copy link
Contributor

This PR adds almost everything we need for the command kops create cluster to make it until the end and launch instances. To do that we added Scaleway to all cloud provider switches and added the model and task for the etcd volumes.

The load-balancer part is not implemented yet so at the very end the GetApiIngressStatus function just returns nil, it will be implemented in a later PR.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 23, 2022
@k8s-ci-robot k8s-ci-robot added area/api area/provider/scaleway Issues or PRs related to Scaleway provider needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 23, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @Mia-Cross. Thanks for your PR.

I'm waiting for a kubernetes 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.

@hakman
Copy link
Member

hakman commented Nov 23, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 23, 2022
@hakman hakman removed the request for review from rdrgmnzs November 23, 2022 18:10
@k8s-ci-robot k8s-ci-robot added area/nodeup needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 23, 2022
@Mia-Cross Mia-Cross force-pushed the scaleway_create_cluster branch from 73cada7 to 160831d Compare November 28, 2022 11:11
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 28, 2022
@Mia-Cross Mia-Cross force-pushed the scaleway_create_cluster branch from 160831d to b49f6c0 Compare November 30, 2022 14:46
@Mia-Cross Mia-Cross marked this pull request as ready for review November 30, 2022 14:56
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2022
@Mia-Cross
Copy link
Contributor Author

Note : we need to wait for the next release of the etcd-manager with Scaleway support for it to work properly.

@Mia-Cross Mia-Cross force-pushed the scaleway_create_cluster branch 2 times, most recently from 47d0826 to 83a87f0 Compare December 1, 2022 16:31
hakman
hakman requested changes Dec 1, 2022
View reviewed changes
@Mia-Cross Mia-Cross force-pushed the scaleway_create_cluster branch from 83a87f0 to 2f071d6 Compare December 2, 2022 12:52
@Mia-Cross Mia-Cross mentioned this pull request Dec 2, 2022
Merged
@Mia-Cross
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2022
hakman
hakman requested changes Dec 5, 2022
View reviewed changes
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

Looks mostly good, few nits to address.

upup/pkg/fi/cloudup/scalewaytasks/instance.go
@@ -75,6 +75,37 @@ func (s *Instance) Run(c *fi.Context) error {
return fi.DefaultDeltaRunMethod(s, c)
Copy link
Member

Choose a reason for hiding this comment

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

Why is the file called "instance"? I thought the Scaleway terminology is "server"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically you're right, "instance" is the name of our virtual machine product so it is related to servers, volumes, images and snapshots but we use interchangeably the terms server and instance. Would you prefer the term server ?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer which one you think it's more appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

...but please consistently use the same term for the same concept.

@Mia-Cross Mia-Cross force-pushed the scaleway_create_cluster branch from 2f071d6 to 7247fdc Compare December 5, 2022 13:40
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2022
hakman
hakman approved these changes Dec 5, 2022
View reviewed changes
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

Cool. Thanks @Mia-Cross!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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 Dec 5, 2022
@k8s-ci-robot k8s-ci-robot merged commit 912bebb into kubernetes:master Dec 5, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Dec 5, 2022
@Mia-Cross Mia-Cross deleted the scaleway_create_cluster branch December 7, 2022 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remyleone remyleone remyleone left review comments

@hakman hakman hakman approved these changes

@johngmyers johngmyers Awaiting requested review from johngmyers

@KashifSaadat KashifSaadat Awaiting requested review from KashifSaadat

Assignees

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/nodeup area/provider/scaleway Issues or PRs related to Scaleway provider 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.
5 participants
@Mia-Cross @k8s-ci-robot @hakman @remyleone @johngmyers