wait.query and wait.port resources #334
Conversation
| wait.port "8080" { | ||
| host = "localhost" | ||
| port = 8080 | ||
| protocol = "tcp" |
There was a problem hiding this comment.
this isn't in the docs, which means it's not in your code, right?
There was a problem hiding this comment.
whoops, I'll remove it. I added that before I realized that udp checks are not really feasible.
| @@ -1,7 +1,7 @@ | |||
| # -*- mode: ruby -*- | |||
| # vi: set ft=ruby : | |||
|
|
|||
| converge_version = "0.2.0" | |||
| converge_version = "0.3.0" | |||
There was a problem hiding this comment.
what's this for? We haven't released 0.3.0 yet.
There was a problem hiding this comment.
I figured it was better to set that now since the examples are built to download the release binaries and will no longer work with the 0.2.0 binary. All in all, I think we need a better system for the examples. I will open an issue on that.
| fi | ||
| [[ "$SECONDS" -ge "$MAX_SECONDS" ]] && exit 1 | ||
| done | ||
| wait.query "elasticsearch-wait" { |
|
|
||
| alive, err := p.checkConnection() | ||
| if err != nil { | ||
| return p, errors.Wrapf(err, "failed to check connection") |
There was a problem hiding this comment.
shouldn't this be more like "connection was not open by the given timeout"? The code did check the connection, it just potentially timed out.
There was a problem hiding this comment.
actually this means something other than a network error (net.OpErr) occurred. I don't have an example of what this may be at this point as I've never seen anything else yet and don't see any other error types being returned after a cursory look through the stdlib code. If the connection fails b/c of an OpErr, checkConnection returns false and nil for the error.
| Host string `hcl:"host"` | ||
|
|
||
| // the TCP port to attempt to connect to. | ||
| Port int `hcl:"port" required:"true"` |
There was a problem hiding this comment.
doesn't this need to be non-zero? Should that check be added?
| // string. A duration string is a possibly signed sequence of decimal numbers, | ||
| // each with optional fraction and a unit suffix, such as "300ms", "-1.5h" or | ||
| // "2h45m". Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h". | ||
| Interval string `hcl:"interval" doc_type:"duration string"` |
There was a problem hiding this comment.
what are the default values for these? What happens if this is an empty string?
| GracePeriod string `hcl:"grace_period" doc_type:"duration string"` | ||
|
|
||
| // the maximum number of attempts before the wait fails. | ||
| MaxRetry int `hcl:"max_retry"` |
There was a problem hiding this comment.
I don't see a default for this either. Doesn't that make it zero?
There was a problem hiding this comment.
interval and maxretry defaults are set in the retrier if they are 0 values. I will update the doc strings to indicate what the defaults are
|
issues addressed and rebased. ready for review again |
| p.Status.AddMessage(fmt.Sprintf("Passed after %d retries (%v)", p.RetryCount, p.Duration)) | ||
| } | ||
| } else { | ||
| p.RaiseLevel(resource.StatusWillChange) |
There was a problem hiding this comment.
why does this mean the resource will change?
There was a problem hiding this comment.
The desired state is that the port is listening. So when you run plan, StatusWillChange tells you that, optimistically, the port will be listening after the apply. Also, it gives you the color in the converge output that lets the user more easily see whether or not the port is available during the plan phase.
That said, I don't think leaving the status as StatusNoChange will affect the functionality outside of the converge output. Do you think that is preferred?
There was a problem hiding this comment.
It's kind of an edge case of the change semantics, isn't it? I think either will work, but could you maybe add a comment to that effect above it?
wait.query and wait.port resources
fixes #238