-
Notifications
You must be signed in to change notification settings - Fork 34
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
Wait until deploy completes #58
Conversation
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.
thank you for submitting!
Dockerfile
Outdated
|
||
# kubectl 1.7.0 | ||
#FROM google/cloud-sdk:163.0.0-alpine | ||
FROM google/cloud-sdk:163.0.0-alpine |
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.
they're up to 182 now; any reason to not use that? (I don't know the history of why these are both here)
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.
I didn't find a build for 182, so I changed it to 181.
https://hub.docker.com/r/google/cloud-sdk/builds/
I don't know the history of this either.
main.go
Outdated
@@ -126,6 +128,17 @@ func wrapMain() error { | |||
Usage: "Git tag", | |||
EnvVar: "DRONE_TAG", | |||
}, | |||
cli.StringSliceFlag{ | |||
Name: "rollout_check", |
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.
can this be called wait_deployments or something like that, to make it obvious that the values are Deployments?
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.
It's not necessary deployments, it could be also daemonsets or statefulsets.
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.
not the way you've set it up and documented it here
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.
I haven't tried, but I think this should work:
rollout_check: [gateway-server, deployment/gateway-server, daemonsets/some-set]
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.
that sounds great, but deployment
is hardcoded in the command below (L439)
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.
Right, I see. I'll do it for deployment
only for now.
main.go
Outdated
EnvVar: "PLUGIN_ROLLOUT_CHECK", | ||
}, | ||
cli.IntFlag{ | ||
Name: "rollout_timeout", |
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.
similarly, wait_seconds
might want to mention that exceeding the timeout fails the build
main.go
Outdated
"rollout", | ||
"status", | ||
"deploy", | ||
deployment, |
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.
I think the output from running these in series could be confusing -- I would wonder why the other deployments aren't showing up. maybe adding & to the end of the command, or otherwise running in parallel, would be more intuitive.
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.
We should tackle this in a follow up.
Adding &
would run them in parallel, however mix up the log messages (that might not matter if the end result includes the name deployment "x" successfully rolled out
).
IMO checking that the deployment succeeded should occur in another step.
This allows the user to configure run as many steps as there are deployments, and group these steps so they run in parallel.
Of course the downside of this is the number of resources being rolled out in each kind of event would need to be known ahead of time (unless that step was a configurable plugin that could parse .kube.yml
, or something).
Created issue to track here: #64.
main.go
Outdated
if rolloutTimeout == 0 { | ||
err = runner.Run(kubectlCmd, command[3:]...) | ||
} else { | ||
err = runner.Run(timeoutCmd, command...) |
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.
not that big of a deal, but I find all this handling of the command a little confusing.
Updated code.
Example output:
|
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 is great, thanks again.
would you mind adding this info to DOCS.md?
Sure, updated |
main.go
Outdated
counterProgress = fmt.Sprintf("[%d/%d]", counter + 1, waitDeploymentsCount) | ||
} | ||
|
||
log(fmt.Sprintf("%sWaiting till rollout completes for %s\n", counterProgress, deployment)) |
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.
"Waiting until rollout completes for Deployment 1/2"
Otherwise this will get conflated with the output of the command itself
DOCS.md
Outdated
@@ -13,6 +13,8 @@ The following parameters are used to configure this plugin: | |||
* *optional* `namespace` - Kubernetes namespace to operate in (defaults to `default`) | |||
* *optional* `template` - Kubernetes manifest template (defaults to `.kube.yml`) | |||
* *optional* `secret_template` - Kubernetes [_Secret_ resource](http://kubernetes.io/docs/user-guide/secrets/) manifest template (defaults to `.kube.sec.yml`) | |||
* *optional* `wait_deployments` - list of deployments to check after apply |
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.
List of Deployments to wait for successful rollout, using kubectl rollout status
DOCS.md
Outdated
@@ -13,6 +13,8 @@ The following parameters are used to configure this plugin: | |||
* *optional* `namespace` - Kubernetes namespace to operate in (defaults to `default`) | |||
* *optional* `template` - Kubernetes manifest template (defaults to `.kube.yml`) | |||
* *optional* `secret_template` - Kubernetes [_Secret_ resource](http://kubernetes.io/docs/user-guide/secrets/) manifest template (defaults to `.kube.sec.yml`) | |||
* *optional* `wait_deployments` - list of deployments to check after apply | |||
* *optional* `wait_seconds` - if `wait_deployments` is set, maximum number of seconds for rollout (the exceeded timeout сauses the build to fail) |
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.
number of seconds to wait before failing the build
Is there anything else I could do? |
you mean to get this merged? I added some more reviewers. Note that I'm pretty sure this will only apply to drone.dv.nyt.net; not the old ones. |
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.
I am approving for people who want this functionality now.
Merging to develop will make this available in nytimes/drone-gke:develop
.
Will need to tag a 0.7.x
release for this as well.
Dockerfile
Outdated
@@ -1,8 +1,8 @@ | |||
# kubectl 1.6.6 | |||
FROM google/cloud-sdk:161.0.0-alpine | |||
#FROM google/cloud-sdk:161.0.0-alpine |
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.
can you delete these two lines?
Dockerfile
Outdated
@@ -1,8 +1,8 @@ | |||
# kubectl 1.6.6 | |||
FROM google/cloud-sdk:161.0.0-alpine | |||
#FROM google/cloud-sdk:161.0.0-alpine | |||
|
|||
# kubectl 1.7.0 |
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.
Can you update this version number?
Edit on my comment above: will need to tag a |
Related to #26
Example usage:
Example output:
There are no checks for deamonset or statefulset, but it would be easy to add this functionality.