-
Notifications
You must be signed in to change notification settings - Fork 135
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
R ovh domains zone #3
Conversation
FYI, i'm currently working with ovh to get a free ovh zone so that we can merge this PR and run the tests on HC infrastructure. |
Hi @yanndegat , |
yes but i'm not able to ask hashicorp to do so
…-yann
On Tue, Jul 25, 2017 at 2:32 PM, Rémi Jouannet ***@***.***> wrote:
Hi @yanndegat <https://github.com/yanndegat> ,
no problem i'm not in the hurry,
you also can buy really cheap domain with the ext .ovh
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/terraform-providers/terraform-provider-ovh/pull/3#issuecomment-317722905>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQcpparrIkxB9zPi9_AQNoOjCXh0bzGtks5sReBZgaJpZM4ONuTS>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing aside, ran through a quick review.
Type: schema.TypeString, | ||
Required: true, | ||
}, | ||
"ttl": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason ttl
needs to be a TypeString
and not a TypeInt
? Would save you from having to call strconv
during Create.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@remijouannet ping ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, saw you changed it to TypeInt
😄
ovh/resource_ovh_dns_record.go
Outdated
Target: d.Get("target").(string), | ||
} | ||
|
||
newRecord.Ttl, _ = strconv.Atoi(d.Get("ttl").(string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ttl
has to be a string, dropping the error here.
ovh/resource_ovh_dns_record.go
Outdated
} | ||
|
||
d.SetId(strconv.Itoa(resultRecord.Id)) | ||
d.Set("id", strconv.Itoa(resultRecord.Id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate set to id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grubernaut as i understand terraform generate internally an ID for every ressource,
the OVH api return a ID for each DNS entry so I force the internal terraform ID to be the ID return by the API
as this id is also part of the resource i'm setting it in the resource data schema
I don't know if i'm clear here and if this right way to do it
ovh/resource_ovh_dns_record.go
Outdated
return fmt.Errorf("Failed to create OVH Record: %s", err) | ||
} | ||
|
||
d.SetId(strconv.Itoa(resultRecord.Id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use fmt.Sprintf("%d", resultRecord.Id))
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there really a difference between using Sprintf and strconv ?
ovh/resource_ovh_dns_record.go
Outdated
func resourceOVHRecordRead(d *schema.ResourceData, meta interface{}) error { | ||
provider := meta.(*Config) | ||
|
||
recordID, err := strconv.Atoi(d.Id()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't believe you actually need this conversion back to an int
here.
ovh/resource_ovh_dns_record.go
Outdated
|
||
record := Record{} | ||
err = provider.OVHClient.Get( | ||
fmt.Sprintf("/domain/zone/%s/record/%d", d.Get("zone").(string), recordID), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can just use %s
for the id
here as you're building a string.
ovh/resource_ovh_dns_record.go
Outdated
d.Set("zone", record.Zone) | ||
d.Set("fieldType", record.FieldType) | ||
d.Set("subDomain", record.SubDomain) | ||
d.Set("ttl", strconv.Itoa(record.Ttl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with ttl
here
ovh/resource_ovh_dns_record.go
Outdated
func resourceOVHRecordUpdate(d *schema.ResourceData, meta interface{}) error { | ||
provider := meta.(*Config) | ||
|
||
recordID, err := strconv.Atoi(d.Id()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can leave this as a string
return nil | ||
} | ||
|
||
d.Set("id", record.Id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
record.Id
is an int
, can't use it for d.Set()
. Need to use fmt.Sprintf("%d", record.Id)
or strconv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've changed the type of id in the data schema
ovh/resource_ovh_dns_record.go
Outdated
|
||
log.Printf("[INFO] Deleting OVH Record: %s.%s, %s", d.Get("zone").(string), d.Get("subDomain").(string), d.Id()) | ||
|
||
recordID, err := strconv.Atoi(d.Id()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can skip type conversion here, and continue using as a string on line 174
thx for the review, will look at it as soon as I have free time for it |
@yanndegat @grubernaut i made a few change following your comments, ttl and id type mostly |
@yanndegat @grubernaut Hello, any news ? |
Hey @remijouannet, sorry for the delay. Looking now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor nit
ttl = "3600" | ||
target = "0.0.0.0" | ||
} | ||
`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing '`' here
Hey @remijouannet, looks good, thanks for the contribution! |
@grubernaut still in discussion with OVH for a free domain for your acceptance tests ? |
I believe @yanndegat was in conversations with them, but if they are unable to provide one to us for free for acceptance testing, or require our input to do so, we can help out there. If they're fully unable to provide one to us for free testing, @randomcamel should be able to get us a paid account for it. |
@grubernaut @randomcamel do you already have an account for the openstack part ? doing all the test with the same account should be more convenient, an .ovh domain is very cheap like 3e/year, it should be enought for the acceptance tests |
@remijouannet Aye, we currently have an OVH account that we test with, just need the person with the credit card (cough cough @randomcamel) to add a domain 😄 |
hi currently on holidays |
Hey @yanndegat, Thanks for the patience in getting this in! We're currently working alongside OVH to get a domain for testing purposes, but are running into some issues there. Stay tuned for more! |
hey @yanndegat @grubernaut just want to know the status of the discussion with OVH |
heyhey @yanndegat @grubernaut just passing by to remind you the existence of this PR |
Hey @remijouannet, I no longer work for HashiCorp, but @catsby does and I believe @randomcamel is still the Terraform manager, they should be able to get an OVH domain setup for testing purposes so this PR can finally be merged. Thanks!! |
and from my side, i work for ovh now, and i should be able to help for this
setup, and get a voucher for this purpose.
…-yann
On Tue, Oct 31, 2017 at 7:02 PM, Jake Champlin ***@***.***> wrote:
Hey @remijouannet <https://github.com/remijouannet>, I no longer work for
HashiCorp, but @catsby <https://github.com/catsby> does and I believe
@randomcamel <https://github.com/randomcamel> is still the Terraform
manager, they *should* be able to get an OVH domain setup for testing
purposes so this PR can finally be merged. Thanks!!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/terraform-providers/terraform-provider-ovh/pull/3#issuecomment-340850614>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQcppT-cxmybiCnBGGhgySp-lQc3z_JCks5sx2C6gaJpZM4ONuTS>
.
|
This feature would be very usefull.
Refresh request: |
hello @tounefr fix in this pull request as well |
Hello @catsby @randomcamel, |
hi,
@catsby @randomcamel
i can get you a voucher for this
if you give me the ovh account that is registered for hashicorp i can even
make all the setup for you
regards
…-yann
On Tue, Nov 7, 2017 at 2:16 PM, Rémi Jouannet ***@***.***> wrote:
Hello @catsby <https://github.com/catsby> @randomcamel
<https://github.com/randomcamel>,
it's been a 5 month since this PR is open,
the code has been reviewed, fix and validate,
can you please get a OVH domain for the acceptance tests ***@***.***
<https://github.com/yanndegat> can help us for that)
so this PR can be merge
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/terraform-providers/terraform-provider-ovh/pull/3#issuecomment-342478894>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQcppbtQQfYU5xY1A7s2dNMOt3BbWUEeks5s0FgwgaJpZM4ONuTS>
.
|
I'm using this branch after rebase with latest master and it's working perfectly so far. |
I've been working with @yanndegat to allow the CI system to be able to run these tests - this has now been completed and the tests can run (and pass!): |
@tombuildsstuff @yanndegat awesome thx for your work |
thanks for your time and your patience!
and thank you once again @tombuildsstuff
…-yann
On Tue, Dec 5, 2017 at 1:48 PM, Rémi Jouannet ***@***.***> wrote:
@tombuildsstuff <https://github.com/tombuildsstuff> @yanndegat
<https://github.com/yanndegat> awesome thx for your work
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/terraform-providers/terraform-provider-ovh/pull/3#issuecomment-349294435>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQcppSUaSF-PDAUQTal3pFwcdWm1F3kSks5s9TudgaJpZM4ONuTS>
.
|
Hello @yanndegat
As you ask in the previous pr hashicorp/terraform#14775
i moved my pr in this project,
here is my little readme again so we can continue to review my pr here
howto
if you want to use the OVH API you have to generate:
An Application Key
An Application Secret Key
A Consumer Key
you can generate all three on this page
https://eu.api.ovh.com/createToken/
the following rights are needed for this plugin
GET : /domain/zone/*
PUT : /domain/zone/*
POST : /domain/zone/*
DELETE : /domain/zone/*
GET : /me
ovh.tf example
provider "ovh" {
application_key = "azrzqrgqvvdsfgsfffgc"
application_secret = "aztfqsqfsdcsdqezrfdvcx"
consumer_key = "aergfvdsrgtfbvcretfgd"
}
resource "ovh_domain_zone_record" "test" {
zone = "testdemo.ovh"
subDomain = "test"
fieldType = "A"
ttl = "3600"
target = "0.0.0.0"
}
testacc
export OVH_APPLICATION_KEY="arqzergfazfeef"
export OVH_APPLICATION_SECRET="aertedytfghftyhytghbgfv"
export OVH_CONSUMER_KEY="tyfghjhuyghvbjhuytghbjygh"
export OVH_ZONE="testdemo.ovh"
then use the makefile : make testacc TEST=./builtin/providers/ovh