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 measure_latency option to Route 53 Health Check resource. #3688

Merged
merged 2 commits into from
Jan 7, 2016

Conversation

ajvb
Copy link
Contributor

@ajvb ajvb commented Oct 29, 2015

Adds measure_latency option to Route 53 Health Check resource.

This allows for the health check to record request latency and graph it within the Route 53 UI.

Related to #3273

@@ -128,6 +133,10 @@ func resourceAwsRoute53HealthCheckCreate(d *schema.ResourceData, meta interface{
healthConfig.ResourcePath = aws.String(v.(string))
}

if v, ok := d.GetOk("measure_latency"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Schema elements with a Default value do not need to be checked with GetOk; it will always be ok. Here, healthConfig.MeasureLatency = aws.Bool(d.Get("measure_latency").(bool)) is fine.

Also, since this is a boolean, if the user specified false in their configuration this block would never be entered. since GetOk checks for non-default values. Not that you should have known these things, just mentioning it 😄

@catsby
Copy link
Contributor

catsby commented Oct 29, 2015

Hey @ajvb thanks for the PR

Looking at the documentation, I think we're missing a few things:

  • when the health check value of Type is CALCULATED, we need to omit this element
  • we can't change the value of MeasureLatency after the health check has been created, do that element needs to be a ForceNew

@catsby catsby added enhancement waiting-response An issue/pull request is waiting for a response from the community provider/aws labels Oct 29, 2015
@catsby
Copy link
Contributor

catsby commented Oct 29, 2015

It would also be create if we could check it's existence and value in TestAccAWSRoute53HealthCheck_basic, even if just in resource.TestCheckResourceAttr()

@ajvb
Copy link
Contributor Author

ajvb commented Oct 29, 2015

@catsby Thank you for the feedback! Let me make those changes and push it up.

@ajvb
Copy link
Contributor Author

ajvb commented Oct 29, 2015

@catsby This also seems to be the case for the resource_path field, but I can't seem to find where it does that checking.

@catsby
Copy link
Contributor

catsby commented Oct 29, 2015

It doesn't seem to be a thing that's currently being checked 😄

* Added ignoring of param when Type is CALCULATED
* Added ForceNew param to measure_latency item in schema
* Added check to test
@ajvb
Copy link
Contributor Author

ajvb commented Oct 30, 2015

@catsby Ok awesome, is there any else required of me before this gets merged in?

@catsby catsby removed the waiting-response An issue/pull request is waiting for a response from the community label Jan 7, 2016
@catsby
Copy link
Contributor

catsby commented Jan 7, 2016

Hey @ajvb – terribly sorry for the delay here, I'm going to merge this in now. Thanks!

catsby added a commit that referenced this pull request Jan 7, 2016
Added measure_latency option to Route 53 Health Check resource.
@catsby catsby merged commit 829fffc into hashicorp:master Jan 7, 2016
@ghost
Copy link

ghost commented Apr 29, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants