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

Hetzner support #1043

Merged
merged 6 commits into from
Jul 8, 2021
Merged

Hetzner support #1043

merged 6 commits into from
Jul 8, 2021

Conversation

poelzi
Copy link
Contributor

@poelzi poelzi commented Jun 24, 2021

What this PR does / why we need it:

This PR adds support for the hetzner provider extension

Special notes for your reviewer:

hcloud provider extension
hcloud machine controller

Release note:

* Support for the hetzner cloud extension (hcloud)

@gardener-robot
Copy link

@poelzi Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jun 24, 2021
@gardener-robot-ci-1
Copy link
Contributor

Thank you @poelzi for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@gardener-robot
Copy link

@holgerkoser, @grolu You have pull request review open invite, please check

@grolu
Copy link
Contributor

grolu commented Jun 30, 2021

Hi @poelzi,
thank you for your contribution. Could you please adapt the header in your files to our standard header

<!--
SPDX-FileCopyrightText: 2021 SAP SE or an SAP affiliate company and Gardener contributors

SPDX-License-Identifier: Apache-2.0
 -->

By merging your request we take over ownership and responsibility for your code, therefore the headers have to be adapted.

Thank you

@gardener-robot
Copy link

@grolu You have pull request review open invite, please check

@@ -120,6 +120,9 @@ export default {
return findFreeNetworks(this.currentZonesNetworkConfiguration, this.existingWorkerCIDR, this.cloudProviderKind, this.allZones.length)
},
availableZones () {
if (!this.zonedCluster) {
Copy link
Contributor

Choose a reason for hiding this comment

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

One last question before we merge your PR: Why did you add this code here? While I don't think that it will cause any problem with other infrastructures, I'm curious why you added it in the first place. I briefly tested your implementation and for me it worked without errors even without this code (at least not on the create cluster screen). Did you experience an error when changing an existing worker or did you just add this to be on the safe side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the hetzner provider implemenation was first without zones and the frontend assumes zones so i fixed some issues in this regard. We later switched to zones and the fixes became obsolete. I think they are still useful so I let them in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for clarification: When did the issue occur? After shoot creation on the configure workers popup? Or somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When creating a now shoot, it complained about missing zones even the api returned no zones. I added the zonedCluster to return false in hcloud case and with this fix the problem was gone.

Copy link
Contributor

@grolu grolu left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Jul 6, 2021
@petersutter petersutter self-requested a review July 6, 2021 12:15
Copy link
Member

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

/lgtm

@petersutter petersutter added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added the needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2021
@holgerkoser holgerkoser merged commit 8cd7441 into gardener:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants