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

Added Disabled to Route 53 health check resource #8939

Conversation

Glen-Moonpig
Copy link
Contributor

@Glen-Moonpig Glen-Moonpig commented Jun 11, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8937
Closes #10117

Release note for CHANGELOG:

resource/aws_route53_health_check: Added disabled argument [GH-8937]

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/route53 Issues and PRs that pertain to the route53 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jun 11, 2019
@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Jun 11, 2019
@aeschright aeschright requested a review from a team June 28, 2019 18:13
@ghost ghost added the documentation Introduces or discusses updates to documentation. label Dec 6, 2019
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @Glen-Moonpig 👋 This is off to a great start -- we just need to introduce covering acceptance testing that verifies it can be disabled on creation and that updates occur as expected. Please reach out if you have any questions or do not have time to implement the feedback. 👍

@@ -279,6 +279,7 @@ resource "aws_route53_health_check" "foo" {
request_interval = "30"
measure_latency = true
invert_healthcheck = true
disabled = false
Copy link
Contributor

Choose a reason for hiding this comment

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

To ensure we are not introducing any regressions (unlikely given the implementation here, but as a good practice), could we leave the existing test configurations without these changes and introduce a new acceptance test with its own configuration that verifies the new functionality? e.g.

func TestAccAWSRoute53HealthCheck_Disabled(t *testing.T) {
  resourceName := "aws_route53_health_check.test"

  resource.ParallelTest(t, resource.TestCase{
    PreCheck:      func() { testAccPreCheck(t) },
    Providers:     testAccProviders,
    CheckDestroy:  testAccCheckRoute53HealthCheckDestroy,
    Steps: []resource.TestStep{
      {
        Config: testAccRoute53HealthCheckConfigDisabled(true),
        Check: resource.ComposeTestCheckFunc(
          testAccCheckRoute53HealthCheckExists(resourceName),
          resource.TestCheckResourceAttr(resourceName, "disabled", "true"),
        ),
      },
      {
        ResourceName:      resourceName,
        ImportState:       true,
        ImportStateVerify: true,
      },
      {
        Config: testAccRoute53HealthCheckConfigDisabled(false),
        Check: resource.ComposeTestCheckFunc(
          testAccCheckRoute53HealthCheckExists(resourceName),
          resource.TestCheckResourceAttr(resourceName, "disabled", "false"),
        ),
      },
      {
        Config: testAccRoute53HealthCheckConfigDisabled(true),
        Check: resource.ComposeTestCheckFunc(
          testAccCheckRoute53HealthCheckExists(resourceName),
          resource.TestCheckResourceAttr(resourceName, "disabled", "true"),
        ),
      },
    },
  })
}

func testAccRoute53HealthCheckConfigDisabled(disabled bool) string {
  return fmt.Sprintf(`
resource "aws_route53_health_check" "test" {
  disabled          = %[1]t
  failure_threshold = "2"
  fqdn              = "dev.notexample.com"
  port              = 80
  request_interval  = "30"
  resource_path     = "/"
  type              = "HTTP"
}
`, disabled)

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 6, 2019
@bflad
Copy link
Contributor

bflad commented Jan 23, 2020

Hi again 👋 Since we haven't heard back, we are going to close this pull request for now. If anyone is still interested in this change, please submit a new pull request and we will take a fresh look. Thanks.

@bflad bflad closed this Jan 23, 2020
@ghost
Copy link

ghost commented Mar 27, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 27, 2020
@anGie44 anGie44 reopened this Aug 12, 2020
@anGie44 anGie44 closed this Aug 12, 2020
@anGie44 anGie44 reopened this Aug 12, 2020
@anGie44 anGie44 closed this Aug 13, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/route53 Issues and PRs that pertain to the route53 service. size/XS Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
4 participants