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

Add ovh_iploadbalancing_http_route resource #35

Merged
merged 7 commits into from
Jun 14, 2018
Merged

Add ovh_iploadbalancing_http_route resource #35

merged 7 commits into from
Jun 14, 2018

Conversation

eliecharra
Copy link
Contributor

@eliecharra eliecharra commented May 22, 2018

Sorry OVH related doc is only in french ... https://docs.ovh.com/fr/load-balancer/routes/

  • Handle deletion from state when response is 404
  • Add ovh_iploadbalancing_http_route_rule resource
  • Fix acc tests
  • Doc

@yanndegat
Copy link
Collaborator

yanndegat commented May 28, 2018

hi @eliecharra

thanks a lot for this job

i have a few questions:

  • why are you sorry for the docs ? the doc entries you've added seems to be correct no?
  • what's the acc test issue ? do you need any help with that?
  • could you rebase your PR according to the last merges on master ?

thanks again

@eliecharra
Copy link
Contributor Author

eliecharra commented May 28, 2018

why are you sorry for the docs ? the doc entries you've added seems to be correct no?

Doc written in french, docs should be redacted "english first" in mind as french is not so open source compliant imho ... but this is a detail ;-)

what's the acc test issue ? do you need any help with that?

I havent finished writing tests cases yet, maybe I'll need some help afterwards.
Is Travis now configured to use the load balancer instance ?

could you rebase your PR according to the last merges on master ?

Sure !

@eliecharra
Copy link
Contributor Author

@yanndegat Actually acc test are not launched by Travis, did you plan to configure them ?

I'd like to cover this resources with acc test if possible

@yanndegat
Copy link
Collaborator

hi @eliecharra

i have to sync with @tombuildsstuff
it should be ready soon now

@eliecharra
Copy link
Contributor Author

Ok, let me know when evrything is ready, I'll try to run my tests on it.

@yanndegat
Copy link
Collaborator

hi @eliecharra
i don't know what you mean by running your tests on it.
The tests acc are run by hashicorp team city ci. i dont think you have access to their ressources
am i correct?

@eliecharra
Copy link
Contributor Author

acc test should be setup to run on every PR imho.

The openstack provider, for example, is running openlab CI on every pull request https://github.com/terraform-providers/terraform-provider-openstack/pull/261

By running my test on it, I mean on this pull request through a continous integration process.

@yanndegat
Copy link
Collaborator

well the openstack provider is different from other cloud providers in a sense that there's no real cloud provider thus billed infrastructure behind.

imagine you'd made a PR on a compute resource that mines bitcoins...
you have to check your acc tests before i can merge it
otherwise i have to fix them afterwards ( as im currently doing on another PR :( btw )

@eliecharra eliecharra changed the title [WIP] Add ovh_iploadbalancing_http_route resource Add ovh_iploadbalancing_http_route resource Jun 14, 2018
@yanndegat yanndegat merged commit e0580ac into ovh:master Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants