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

ovh: allow to use ovh.conf file #2216

Merged
merged 5 commits into from
Jun 26, 2024
Merged

Conversation

ldez
Copy link
Member

@ldez ldez commented Jun 21, 2024

Fixes #2215

@rbeuque74
Copy link

Hello,

Thanks for the patch. I suggested another way around here: ldez#2

What do you think ?

Romain

@ldez ldez requested a review from dmke June 24, 2024 21:02
@ldez ldez marked this pull request as ready for review June 24, 2024 21:03
@ldez ldez added this to the v4.18 milestone Jun 24, 2024
@dmke
Copy link
Member

dmke commented Jun 25, 2024

I'm a little confused: Where does it read the ovh.conf, and why was ovh/go-ovh#84 closed without merging?

@ldez
Copy link
Member Author

ldez commented Jun 25, 2024

The read of the file is inside the constructors of the client.

https://github.com/ovh/go-ovh/blob/9c5297024e0b551f222c66504a3ffdf7b0cd479b/ovh/configuration.go#L77-L91

https://github.com/ovh/go-ovh/blob/9c5297024e0b551f222c66504a3ffdf7b0cd479b/ovh/ovh.go#L103-L159

My PR was to be able to fill all the fields and then call loadConfig but by playing with constructors we can do the same.

It's not my preferred solution but it works as expected.

Copy link
Member

@dmke dmke left a comment

Choose a reason for hiding this comment

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

It's not my preferred solution but it works as expected.

I concur. It works now as expected, but as soon as OVH changes their client implementation (as likely or unlikely this may be) and the behaviour diverges, we have a problem.

I much prefer ovh/go-ovh#84 (or something similar) to become available, but alas, this seems to be the next best thing...

@ldez ldez enabled auto-merge (squash) June 26, 2024 12:25
@ldez ldez merged commit fa0c05f into go-acme:master Jun 26, 2024
4 checks passed
@ldez ldez deleted the feat/ovh-file-conf branch June 26, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

ovh: allow to use ovh.conf
3 participants