-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Bug 1384018: New "Kubernetes Deployments Support" topic #3255
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
Conversation
0c90110
to
913ebd6
Compare
@ahardin-rh PTAL 🙇♀️ |
|
||
- xref:../../dev_guide/managing_images.adoc#dev-guide-managing-images[image streams] | ||
- xref:../../dev_guide/deployments/deployment_strategies.html#lifecycle-hooks[lifecycle hooks] | ||
- xref:../../dev_guide/deployments/deployment_strategies.html#custom-strategy[Custom deployment strategies] |
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.
Custom is capitalized here, but "image" and lifestyle" are not.
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 capitalized it since we capitalize it in the doc it links to, as the type of strategy. I could go either way here.
$ oc rollout pause deployments/<name> | ||
---- | ||
|
||
At the moment, upstream Deployment do not support ImageChange triggers. A generic triggering |
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.
s/Deployment/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.
Oops I missed this paragraph completely.
---- | ||
|
||
At the moment, upstream Deployment do not support ImageChange triggers. A generic triggering | ||
mechanism has been proposed upstream but it is unknown if and when will it be accepted. |
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.
upstream, but
---- | ||
|
||
At the moment, upstream Deployment do not support ImageChange triggers. A generic triggering | ||
mechanism has been proposed upstream but it is unknown if and when will it be accepted. |
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.
s/will it/it will
|
||
At the moment, upstream Deployment do not support ImageChange triggers. A generic triggering | ||
mechanism has been proposed upstream but it is unknown if and when will it be accepted. | ||
Eventually, we can implement our own mechanism on top but it would be more desirable for it |
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.
maybe reword this sentence to focus on "you" instead of "we". Also, the "on top" is confusing. Perhaps reword to:
Eventually, you will be able to implement your own mechanism. However, it is more desirable to have this exist as part of the Kubernetes core.
[[dc-vs-d-custom-strategies]] | ||
==== Custom Strategies | ||
|
||
Kubernetes deployments do not yet support user-specified Custom 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.
s/Custom/custom
@adellape Just a few comments from me. Looks good overall! |
19a9038
to
fc0547e
Compare
@ahardin-rh Thanks, edits made. @Kargakis @mfojtik PTAL. Preview: Some notes on my thinking here:
|
fc0547e
to
386ebbc
Compare
@@ -107,6 +107,7 @@ syntax: | |||
|`build` | | |||
|`buildConfig` | `bc` | |||
|`deploymentConfig` | `dc` | |||
|`deployments` (Technology Preview)| |
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.
@Kargakis Correct that there's no abbreviation for this?
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.
In 3.3 there is none, in 3.4 there is deploy
.
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.
OK thanks, since this PR is for 3.4, I'll add deploy
as the abbreviation here.
@@ -117,6 +118,7 @@ syntax: | |||
|`pod` |`po` | |||
|`ResourceQuota` | `quota` | |||
|`replicationController` |`rc` | |||
|`replicaSet` (Technology Preview)|`rs` |
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.
@Kargakis Correct to say that replica sets are also Tech Preview, if deployments are?
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 sure about that, @smarterclayton ?
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.
Ah I see replica sets listed as Tech Preview in Origin 1.3 here:
https://github.com/openshift/origin#alpha-and-unsupported-kubernetes-features
image: nginx:1.7.9 | ||
ports: | ||
- containerPort: 80 | ||
---- |
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.
@Kargakis This example is from the Kube deployments doc. Is there another one you'd suggest using instead?
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.
No, it's fine. I would change the name from "nginx-deployment" to "nginx".
LGTM |
386ebbc
to
1c2f0a4
Compare
Latest edits made and QE sign-off in BZ. Merging. |
[rev_history] |
Thanks! |
https://bugzilla.redhat.com/show_bug.cgi?id=1384018
Cherry-picks #3205 and builds on it.
Preview:
http://file.rdu.redhat.com/~adellape/111816/kubernetes-deployments/dev_guide/deployments/kubernetes_deployments.html