Fix deploy timeouts #174
Fix deploy timeouts #174
Conversation
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
| All notable changes to this project will be documented in this file. | |||
| This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|
|||
| ## [NEXT_RELEASE] ## | |||
There was a problem hiding this comment.
- ## [NEXT_RELEASE] ##
+ ## [NEXT_RELEASE]
k8s/deployments.go
Outdated
| } | ||
| fmt.Fprintln(w, "deploy finished with success") | ||
| w.Close() |
There was a problem hiding this comment.
Why not defer w.Close() on start of anonymous function?
k8s/deployments.go
Outdated
| lc.WithError(err).Error("error updating deployment") | ||
| fmt.Fprintln(w, "error when doing rolling update") | ||
| return | ||
| L: |
There was a problem hiding this comment.
What do you think about use Loop: instead of L: for this label ?
Maybe it will help us for a quick understand of this block of code
k8s/deployments.go
Outdated
| case <-time.After(deploymentConfig.KeepAliveTimeout): | ||
| fmt.Fprint(w, keepAliveMessage) | ||
| case <-done: | ||
| break L |
There was a problem hiding this comment.
or you can simplify it using two break, one for select and another for for.
There was a problem hiding this comment.
I'm going to pick two breaks.
aguerra
force-pushed
the
ag-fix-deploy-timeouts
branch
from
126b8b7
to
0e00370
Compare
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
| All notable changes to this project will be documented in this file. | |||
| This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|
|||
| ## [NEXT_RELEASE] | |||
| ### Fixed | |||
| - Deploy timeouts by sending a "ping" to the client | |||
There was a problem hiding this comment.
IMO this isn't clear.
k8s/deployments.go
Outdated
| defer close(done) | ||
|
|
||
| // parsing teresa.yaml and updating app | ||
| if err := parsingTeresaYaml(app, file.Data); err != nil { |
There was a problem hiding this comment.
If parsing teresa.yaml fails, the deployment goroutine shouldn't be created in the first place. Maybe we should try and parse it before creating the deployment goroutine and pass the file.Data to the deployment goroutine.
Also, the name of the function that would parse the teresa.yaml would be something as parseTeresaYaml instead of parsingTeresaYaml.
There was a problem hiding this comment.
As a side effect, we would decrease the this creates code and logic, which would be nice.
There was a problem hiding this comment.
If you guys agree with that, please don't forget to add a defer closing the file.Data outside this goroutine - and sorry for the many comments in series.
There was a problem hiding this comment.
Maybe you have just fixed a strange periodic S3 related bug with this last suggestion 👍
Deploy is a streaming operation and we need the connection open even with the presence of firewalls or load balancers that have idle timeouts. As we don't control go-swagger autogenerated server we send a "ping" to the client every 30 seconds by default (this value may be changed using the TERESADEPLOY_KEEPALIVE_TIMEOUT env var).
aguerra
force-pushed
the
ag-fix-deploy-timeouts
branch
from
0e00370
to
9d876dc
Compare
Deploy is a streaming operation and we need the connection open even with the presence of firewalls or load balancers that have idle timeouts. As we don't control go-swagger autogenerated server we send a "ping" to the client every 30 seconds by default (this value may be changed using the TERESADEPLOY_KEEPALIVE_TIMEOUT env var).
This change is