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

provider/hcloud: Add hcloud discovery #159

Closed
wants to merge 1 commit into from
Closed

provider/hcloud: Add hcloud discovery #159

wants to merge 1 commit into from

Conversation

shortner
Copy link

This Pull Request add the Implementation of the hcloud (Hetzner Cloud) provider.

Usage

Hetzner Cloud:

    provider:       "hcloud"
    api_token:      The Hetzner CLoud API token to use (required)
    label_selector: The label selector to filter servers by
    addr_type:      "private_v4", "public_v4" or "public_v6". Defaults to "private_v4". If multiple private networks are defined, the first will be used.

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 18, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

VoyTechnology
VoyTechnology approved these changes Aug 28, 2020
View reviewed changes
Copy link

@VoyTechnology VoyTechnology left a comment

Choose a reason for hiding this comment

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

LGTM! Exactly what I need right now!

@marco-m
Copy link

marco-m commented Sep 19, 2020

@ashwin-venkatesh @alvin-huang any chance you could review this PR ? Do you have somebody else from HashiCorp to suggest ? Thanks!

@thisisnotashwin
Copy link

Hey @marco-m ,

Thanks for bringing this to my attention. This does look good based on a first pass. Having said that, I'd defer to @dnephin as he is far more familiar with this repo.

alvin-huang
alvin-huang reviewed Sep 21, 2020
View reviewed changes
test/tf/hcloud/versions.tf Outdated
terraform {
required_providers {
hcloud = {
source = "terraform-providers/hcloud"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you pin the provider to a version requirement here? We've had issues in the past of providers updating in the background to latest and failing tests when older resource arguments/attributes get deprecated.

alvin-huang
alvin-huang reviewed Sep 21, 2020
View reviewed changes
test/tf/hcloud/main.tf
@@ -0,0 +1,46 @@
provider "hcloud" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Picking an arbitrary place but running a terraform fmt in the test/tf/hcloud directory will line up the equal signs and make this look nicer 😄

@alvin-huang
Copy link
Contributor

This LGTM but I would like another look from someone on @hashicorp/consul-core in case I missed something.

@alvin-huang alvin-huang requested a review from a team September 21, 2020 16:26
mkeeler
mkeeler reviewed Sep 21, 2020
View reviewed changes
discover.go
@@ -59,6 +60,7 @@ var Providers = map[string]Provider{
"triton": &triton.Provider{},
"vsphere": &vsphere.Provider{},
"packet": &packet.Provider{},
"hcloud": &hcloud.Provider{},
Copy link
Member

Choose a reason for hiding this comment

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

Question for @dnephin or @alvin-huang:

What should the determining factors be to have a provider added to the default provider list? If k8s isn't a default should this new provider be part of the default set? Or maybe k8s should be in the default too?

Copy link
Member

Choose a reason for hiding this comment

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

I think with the switch to Go modules we lost the ability to pull in go-discover without k8s dependencies so maybe we should just put k8s in the default set (or separate out that provider dir into its own go module.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we probably forgot to add k8s into the map at the time? I don't recall a particular reason why it was excluded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually there's some notes in the original PR #55

Copy link
Member

Choose a reason for hiding this comment

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

Yeah at the time we could have per-provider dependencies that wouldn't get used unless you pulled in that package. With Go modules that is no longer the case unless we were to add a go.mod to the k8s provider and version it specifically.

For this PR I see no reason to not make it default if the only reason k8s wasn't was due to dependencies.

@marco-m
Copy link

marco-m commented Sep 22, 2020

Thanks to all the HashiCorp team for the kind and quick reaction :-)
@shortner we are almost there, would you have time to follow up the reviews ?

@shortner
Copy link
Author

Thanks for the review. I have adapted the notes and committed the adjustments.

@svenstaro
Copy link

Any chance this could be merged now? I'm very much looking forward to this. :)

Fixes #73.

mkeeler
mkeeler reviewed Oct 12, 2020
View reviewed changes
test/tf/hcloud/versions.tf
version = "1.21.0"
}
}
required_version = ">= 0.13"
Copy link
Member

Choose a reason for hiding this comment

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

Right now our circle ci configuration is setup to use terraform 0.12. So I have a feeling this will fail.

@alvin-huang Do you think we should update all the tf in this repo to 0.13 and use it or should we roll the version requirements of this PR back to tf 0.12 and do the 0.13 upgrade later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be best to pin it back to 0.12 and we can go through and migrate all the providers to 0.13 config at once at a separate time. @shortner can you make that change?

Copy link

Choose a reason for hiding this comment

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

Friendly ping @shortner. :)

@dnephin dnephin mentioned this pull request Nov 4, 2020
Open
@Thunderbottom Thunderbottom mentioned this pull request Feb 15, 2021
Open
@Thunderbottom
Copy link

Seems like the owner of this pull request is an inactive GitHub user. With all respect to the dev, I have opened a pull request that includes some fixes and enhancements over this PR. I hope it can be merged soon :)

@shortner shortner closed this Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svenstaro svenstaro svenstaro left review comments

@mkeeler mkeeler mkeeler left review comments

@alvin-huang alvin-huang alvin-huang left review comments

@VoyTechnology VoyTechnology VoyTechnology 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.
9 participants
@shortner @hashicorp-cla @marco-m @thisisnotashwin @alvin-huang @svenstaro @Thunderbottom @mkeeler @VoyTechnology