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

fix: avoid setting ssl_certificate and remove it from tests #205

Merged
merged 2 commits into from
Oct 19, 2022
Merged

Conversation

ianaya89
Copy link

Affected Components

  • Resources
  • Test
  • Docs
  • Tooling
  • Other

Pre-Requisites

  • Terraform code is formatted with terraform fmt
  • Go code is formatted with go fmt
  • plan & apply command of demo/main.tf file do not produce diffs

Notes for the Reviewer

  • Avoid setting/getting ssl_certificates property
  • Remove it from docs and fixtures

Resolves #193

Copy link
Member

@pilimartinez pilimartinez left a comment

Choose a reason for hiding this comment

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

LGTM

@ianaya89 ianaya89 merged commit ed46873 into main Oct 19, 2022
@ianaya89 ianaya89 deleted the issue-193 branch October 19, 2022 17:12
@@ -280,13 +280,12 @@ func resourceCheck() *schema.Resource {
"ssl_certificates": {
Type: schema.TypeSet,
Optional: true,
Deprecated: "The property `ssl_certificates` is deprecated and it's ignored by the Checkly Public API. It will be removed in a future version.",
Deprecated: "This property is deprecated and it's ignored by the Checkly Public API. It will be removed in a future version.",
Copy link
Contributor

Choose a reason for hiding this comment

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

This change sadly makes it ambiguous which argument has been deprecated. Really Terraform should be dealing with this nicer, so I've raised an issue upstream: hashicorp/terraform-plugin-sdk#1094

Copy link
Author

Choose a reason for hiding this comment

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

@RealOrangeOne thanks for the feedback, do you think we can make something for now to improve the DX and make it clear?

Copy link
Author

Choose a reason for hiding this comment

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

I guess the original message was better than the new one for user understanding

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.

bug: Deprecated ssl_certificates never reconciles when removed
3 participants