Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Allow creation, update, and retrieval of SSL certificate options #56

Merged

Conversation

justincmoy
Copy link
Contributor

Addresses russellcardullo/terraform-provider-pingdom#69 in underlying library

@justincmoy
Copy link
Contributor Author

Looks like this passed the build but didn't leave a mark on the PR: https://travis-ci.org/github/russellcardullo/go-pingdom/builds/710892067

@russellcardullo thoughts on the note about the backwards-incompatibility of the VerifyCertificate bool?

@russellcardullo
Copy link
Owner

@justincmoy yeah the default of false breaking the server default of true for VerifyCertificate on the API for Pingdom isn't ideal for how we have the library today. I think that would cause people using this to have their checks unknowingly changed.

Ideally what we better I think is if we have all the optional params be *bool, *int etc instead of bool, int. That would simplify some of the handling we have in places like this: https://github.com/russellcardullo/go-pingdom/blob/master/pingdom/check_types.go#L107-L113. Then when sending the request we'd avoid serializing items that aren't set which would let them get their default value.

Maybe since these are new params, did you want to try using *bool and *int? Eventually I'd like to change the other optional params but we can start with just the new ones you have here?

@justincmoy
Copy link
Contributor Author

@russellcardullo That sounds good to me. I've pushed up a commit to this PR and updated the tests for both behaviors.

Copy link
Owner

@russellcardullo russellcardullo left a comment

Choose a reason for hiding this comment

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

Thanks @justincmoy!. This LGTM!

@russellcardullo russellcardullo merged commit 2a42460 into russellcardullo:master Sep 15, 2020
@justincmoy justincmoy deleted the ssl-certificate-verify-pr branch September 15, 2020 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants