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

Adding failover IP address data and resource blocks for lookup and at… #234

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

stevenleadbeater
Copy link

…tachment to existing instances.

NB/ The work covers manipulating existing failover IP addresses only, it does not cover ordering new ones

Additionally bumped terraform-plugin-sdk version to mitigate tests failing due to failure described here: https://issueexplorer.com/issue/hashicorp/terraform-provider-local/75

@yanndegat
Copy link
Collaborator

hi @stevenleadbeater

thank you very much for contributing to the provider.
it seems to be great work.

one point though: it seems that the commit updating the vendor deps brings too much stuff in the provider.
i tried locally to bump the sdk from v2.6.1 to v2.9.0 and i dont get the same go.mod

if you don't mind, i'll make a separate PR bumping to v2.9.0 and you then can rebase your PR on it.

is this ok for you ?

i'll start the review soon

@yanndegat
Copy link
Collaborator

here #235

@stevenleadbeater
Copy link
Author

@yanndegat sure I'm happy to sort that out, it should all be rebased now

@yanndegat
Copy link
Collaborator

hi again @stevenleadbeater

i'm starting to review your PR.

it's seems great. yet there's a flaw in the ovh api representing the failover ip, and my guess is that it will be hard
to map in a proper terraform resource as there's no method to "delete" or "detach" an ip.

so i would propose to rename the resource in cloud_project_ip_failover_attach. if you don't mind.

@stevenleadbeater
Copy link
Author

Change made, please let me know if you want it rebased or if you're happy to just squash and merge

@stevenleadbeater
Copy link
Author

stevenleadbeater commented Feb 8, 2022

Bump, would you like anything else on this? I'm happy to accommodate any further changes, I'm running our builds using this code already

@stevenleadbeater
Copy link
Author

Hi @yanndegat please let me know if you want anything else on this PR. I'm ready to make any changes you feel necessary

@yanndegat
Copy link
Collaborator

hi @stevenleadbeater ,

thanks for this. i'll check this next week. Could you just squash your 2 commits please ?

The doc is missing on your ressources. Do you mind adding them in this pr ?
If you don't have time, then we'll add them in another pr, we'll just have to log an issue to track that they're missing.

thanks again for your great work.

@stevenleadbeater
Copy link
Author

@yanndegat squashed as per your request and documentation has been added. Please let me know if there's anything else. I've not really done any ruby before so I'm not sure if I got all the template references right. I plagiarised what was already done quite heavily so hopefully it's going to work but I don't know how to check that

@stevenleadbeater
Copy link
Author

Hi @yanndegat is there anything else you want on this? Please let me know

@stevenleadbeater
Copy link
Author

Hi @yanndegat I know I keep bugging you, just trying to find out if this is still on your radar. I'll try keep the pings to weekly so just let me know when you have anything you want updated or fixed and I'll get it sorted 😄

@yanndegat
Copy link
Collaborator

hi @stevenleadbeater

thanks again and sorry for all this delay.

LGTM, i'll merge it today.

could you please just rebase your PR on master, instead of merging, to avoid merge commits ?

thanks again

…tachment to existing instances.

NB/ The work covers manipulating existing failover IP addresses only, it does not cover ordering new ones

Renaming to clarify intent of resource being able to move attachments between instances but not delete them

Adding documentation

fixing documentation
@stevenleadbeater
Copy link
Author

Thanks @yanndegat rebased as requested

@yanndegat yanndegat merged commit 4432c49 into ovh:master Mar 4, 2022
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