-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Allow new-app to create test deployments #7010
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
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.
How does one indicate, on a test DC, what post-test tagging to perform?
and, given that, does it make sense to allow someone to specify that tag when they invoke new-app?
$ oc new-app myappIST --as-test --push-tag myProdAppIST
or some such syntax? Since that's the 90% use case for this (stitching together a pipeline)?
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 might make sense - it's one more set of arguments. In our hypothesized split, it definitely belongs on oc deploy / oc run
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, here's my straw man:
oc set deployment-hook --pre --image --env -- COMMAND
oc set build-hook --post-commit --shell ...
oc deploy --and-tag=...
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 you're proposing you cannot set it as part of invoking new-app itself? Given the primary use case for creating a test deployment is so it can tag an image when it passes, it seems like i should be able to do that in a single 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 proposing that, just saying I haven't sorted it out yet and would like to see what the actual "set hooks" command looks like before we do the simpler thing.
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 guess i'm ok w/ that strawman.
Improve deployment config get and describe, add test cases around the flows.
2a5d9eb to
880750e
Compare
|
[test] |
|
Evaluated for origin test up to 880750e |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/793/) |
| routeColumns = []string{"NAME", "HOST/PORT", "PATH", "SERVICE", "LABELS", "INSECURE POLICY", "TLS TERMINATION"} | ||
| deploymentColumns = []string{"NAME", "STATUS", "CAUSE"} | ||
| deploymentConfigColumns = []string{"NAME", "TRIGGERS", "LATEST"} | ||
| deploymentConfigColumns = []string{"NAME", "REVISION", "REPLICAS", "TRIGGERED BY"} |
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 like these more, sup
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.
Also not showing replicas so far has been really disturbing. We need though to show current alongside desired, showing only desired is misleading. We also need to make use of -o wide and put images, containers, and the selector under it. Preparing a pull for that.
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.
Do we have current on status now?
On Wed, Feb 3, 2016 at 8:00 PM, Michail Kargakis notifications@github.com
wrote:
In pkg/cmd/cli/describe/printer.go
#7010 (comment):@@ -36,7 +36,7 @@ var (
projectColumns = []string{"NAME", "DISPLAY NAME", "STATUS"}
routeColumns = []string{"NAME", "HOST/PORT", "PATH", "SERVICE", "LABELS", "INSECURE POLICY", "TLS TERMINATION"}
deploymentColumns = []string{"NAME", "STATUS", "CAUSE"}
- deploymentConfigColumns = []string{"NAME", "TRIGGERS", "LATEST"}
- deploymentConfigColumns = []string{"NAME", "REVISION", "REPLICAS", "TRIGGERED BY"}
Also not showing replicas so far has been really disturbing. We need
though to show current alongside desired, showing only desired is
misleading. We also need to make use of -o wide and put images, containers,
and the selector under it. Preparing a pull for that.—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/7010/files#r51814973.
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.
Unfortunately no, it's #6233
|
Any other comments??? :) |
|
@smarterclayton I didn't have anything else. just the "what args should new-app accept" discussion. |
|
Unrelated to this PR, but what's the plan for |
|
Replace with "rollout", repurpose it to be the "micro service deployment On Thu, Feb 4, 2016 at 10:27 AM, Michail Kargakis notifications@github.com
|
|
I'm going to assume that was LGTM and thus [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/793/) (Image: devenv-rhel7_3332) |
|
Evaluated for origin merge up to 880750e |
|
yes, this LGTM |
|
that was fast |
Improve deployment config get and describe, add test cases around
the flows.
@bparees @Kargakis follow up from test deployments