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>
.
|
| Type: schema.TypeString, | ||
| Required: true, | ||
| }, | ||
| "ttl": { |
There was a problem hiding this comment.
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.
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.
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.
@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.
Can use fmt.Sprintf("%d", resultRecord.Id)) here
There was a problem hiding this comment.
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.
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.
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)) |
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.
Can leave this as a string
| return nil | ||
| } | ||
|
|
||
| d.Set("id", record.Id) |
There was a problem hiding this comment.
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.
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.
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. |
|
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