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

[Synthetics] Support SSL #279

Merged
merged 14 commits into from
Aug 12, 2019
Merged

[Synthetics] Support SSL #279

merged 14 commits into from
Aug 12, 2019

Conversation

btoueg
Copy link
Contributor

@btoueg btoueg commented Aug 7, 2019

  • Add support for ssl tests
  • Add acceptance test
  • Update doc
  • Have subtype defaults to http for api tests
  • Make sure existing TF files won't diff

@nmuesch
Copy link
Contributor

nmuesch commented Aug 9, 2019

Hey @btoueg thanks for the PR! The golang client was released + bumped in this provider. Can you rebase from master and we can work on getting this reviewed?

@ghost ghost added size/XL and removed size/XXL labels Aug 12, 2019
Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks for the PR 🎉. Left a couple of small comments inline.

I did notice that with the example config, two successive plans cause an un-empty diff with:

      options.accept_self_signed: "false" => "1"

Can you look into that? Thanks!

@@ -316,6 +359,9 @@ func newSyntheticsTestFromLocalState(d *schema.ResourceData) *datadog.Synthetics
minLocationFailed, _ := strconv.Atoi(attr.(string))
options.SetMinLocationFailed(minLocationFailed)
}
if attr, ok := d.GetOk("options.accept_self_signed"); ok {
options.SetAcceptSelfSigned(attr.(string) == "true")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to just assign this to attr.(string) Should we also safely type assert this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, can't wrap my head around it. It's value is "1" or "0" in TF 0.11, even though is never set to such value

website/docs/r/synthetics.html.markdown Outdated Show resolved Hide resolved
@btoueg
Copy link
Contributor Author

btoueg commented Aug 12, 2019

two successive plans cause an un-empty diff

Indeed, that's the case in TF 0.11. I've handled it, let me know

@nmuesch
Copy link
Contributor

nmuesch commented Aug 12, 2019

Thanks for the iterations here! Looks good to me, I'll go ahead and merge. Thanks for this 🎉🚀

@nmuesch nmuesch merged commit 8f86f8c into DataDog:master Aug 12, 2019
@btoueg btoueg deleted the btoueg/synthetics/add-ssl-support branch August 13, 2019 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants