-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: Add aws route53 health checks #1984
Conversation
data. We dont want to send it along on the lopts because that identifier is not expecting the same identifier as what is set by the user.
func resourceAwsRoute53HealthCheckCreate(d *schema.ResourceData, meta interface{}) error { | ||
conn := meta.(*AWSClient).r53conn | ||
|
||
// do we need to check if the optional fields existf before adding them? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically, yes.
For instance, if you do not check with d.GetOk
, I believe d.Get
will give you an empty string for string values not found/required. You'll then pass that on to the API as a string pointer with value ""
, and the API will complain that isn't a valid value.
@catsby Those 2 comments should be cleared up now |
healthConfig.ResourcePath = aws.String(v.(string)) | ||
} | ||
|
||
wait := resource.StateChangeConf{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't have a refresh func attached to it anymore, do this do anything?
I'm getting a panic when running either of the supplied tests, looks like some more work is needed here:
|
Optional: true, | ||
Set: schema.HashString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for this change? schema.HashString
is a newer thing, shortcut for the Set: func
you have above. Is this from a bad merge/rebase maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why are we changing it back to func
in this pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Fixed.
@catsby I've fixed up the create to remove the wait altogether. You were right, we didn't need that for this call. |
I would love to see this merged. Let me know if anything needs to be done for that to happen. |
We're using this internally on a fork of terraform. Would love to see it merged or additional feedback if needed. |
Should we close this in favor of #2226? @nicgrayson @rubbish |
#2226 seems to omit @nicgrayson from the contributors, if he's OK with that then I think we should move forward with it |
@catsby That is fine, I just want it merged 😄 |
Closing in favor of #2226 |
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. |
This PR adds a resource for route53 health checks.