Skip to content

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.

Sign up for GitHub

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

Merged
openshift-bot merged 1 commit into from
Feb 4, 2016
Merged

Allow new-app to create test deployments #7010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions contrib/completions/bash/oc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,7 @@ _oc_new-app()
flags_completion=()

flags+=("--allow-missing-images")
flags+=("--as-test")
flags+=("--code=")
flags+=("--context-dir=")
flags+=("--docker-image=")
Expand Down
1 change: 1 addition & 0 deletions contrib/completions/bash/openshift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5646,6 +5646,7 @@ _openshift_cli_new-app()
flags_completion=()

flags+=("--allow-missing-images")
flags+=("--as-test")
flags+=("--code=")
flags+=("--context-dir=")
flags+=("--docker-image=")
Expand Down
1 change: 1 addition & 0 deletions pkg/cmd/cli/cmd/newapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ func NewCmdNewApplication(fullName string, f *clientcmd.Factory, out io.Writer)
},
}

cmd.Flags().BoolVar(&config.AsTestDeployment, "as-test", config.AsTestDeployment, "If true create this application as a test deployment, which validates that the deployment succeeds and then scales down.")
Copy link
Contributor

@bparees bparees Feb 3, 2016

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)?

Copy link
Contributor Author

@smarterclayton smarterclayton Feb 3, 2016

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

Copy link
Contributor Author

@smarterclayton smarterclayton Feb 4, 2016

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=...

Copy link
Contributor

@bparees bparees Feb 4, 2016

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.

Copy link
Contributor Author

@smarterclayton smarterclayton Feb 4, 2016

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.

Copy link
Contributor

@bparees bparees Feb 4, 2016

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.

cmd.Flags().StringSliceVar(&config.SourceRepositories, "code", config.SourceRepositories, "Source code to use to build this application.")
cmd.Flags().StringVar(&config.ContextDir, "context-dir", "", "Context directory to be used for the build.")
cmd.Flags().StringSliceVarP(&config.ImageStreams, "image", "", config.ImageStreams, "Name of an image stream to use in the app. (deprecated)")
Expand Down
44 changes: 40 additions & 4 deletions pkg/cmd/cli/describe/printer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Copy link
Contributor

@0xmichalis 0xmichalis Feb 4, 2016

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

Copy link
Contributor

@0xmichalis 0xmichalis Feb 4, 2016

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.

Copy link
Contributor Author

@smarterclayton smarterclayton Feb 4, 2016

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.

Copy link
Contributor

@0xmichalis 0xmichalis Feb 4, 2016

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

templateColumns = []string{"NAME", "DESCRIPTION", "PARAMETERS", "OBJECTS"}
policyColumns = []string{"NAME", "ROLES", "LAST MODIFIED"}
policyBindingColumns = []string{"NAME", "ROLE BINDINGS", "LAST MODIFIED"}
Expand Down Expand Up @@ -452,18 +452,54 @@ func printRouteList(routeList *routeapi.RouteList, w io.Writer, opts kctl.PrintO
}

func printDeploymentConfig(dc *deployapi.DeploymentConfig, w io.Writer, opts kctl.PrintOptions) error {
var scale string
if dc.Spec.Test {
scale = fmt.Sprintf("%d (during test)", dc.Spec.Replicas)
} else {
scale = fmt.Sprintf("%d", dc.Spec.Replicas)
}

containers := sets.NewString()
if dc.Spec.Template != nil {
for _, c := range dc.Spec.Template.Spec.Containers {
containers.Insert(c.Name)
}
}
//names := containers.List()
referencedContainers := sets.NewString()

triggers := sets.String{}
for _, trigger := range dc.Spec.Triggers {
triggers.Insert(string(trigger.Type))
switch t := trigger.Type; t {
case deployapi.DeploymentTriggerOnConfigChange:
triggers.Insert("config")
case deployapi.DeploymentTriggerOnImageChange:
if p := trigger.ImageChangeParams; p != nil && p.Automatic {
var prefix string
if len(containers) != 1 && !containers.HasAll(p.ContainerNames...) {
sort.Sort(sort.StringSlice(p.ContainerNames))
prefix = strings.Join(p.ContainerNames, ",") + ":"
}
referencedContainers.Insert(p.ContainerNames...)
switch p.From.Kind {
case "ImageStreamTag":
triggers.Insert(fmt.Sprintf("image(%s%s)", prefix, p.From.Name))
default:
triggers.Insert(fmt.Sprintf("%s(%s%s)", p.From.Kind, prefix, p.From.Name))
}
}
default:
triggers.Insert(string(t))
}
}
tStr := strings.Join(triggers.List(), ", ")
trigger := strings.Join(triggers.List(), ",")

if opts.WithNamespace {
if _, err := fmt.Fprintf(w, "%s\t", dc.Namespace); err != nil {
return err
}
}
_, err := fmt.Fprintf(w, "%s\t%s\t%v\n", dc.Name, tStr, dc.Status.LatestVersion)
_, err := fmt.Fprintf(w, "%s\t%v\t%s\t%s\n", dc.Name, dc.Status.LatestVersion, scale, trigger)
return err
}

Expand Down
9 changes: 6 additions & 3 deletions pkg/cmd/cli/describe/projectstatus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,13 +247,16 @@ func TestProjectStatus(t *testing.T) {
"svc/database - 172.30.17.240:5434 -> 3306",
"exposed by route/frontend on pod port 8080",
"svc/frontend - 172.30.17.154:5432 -> 8080",
"database deploys",
"database test deploys",
"frontend deploys",
"with docker.io/centos/ruby-22-centos7:latest",
"#3 deployment pending on image",
"#2 deployment failed less than a second ago: unable to contact server - 0/1 pods",
"#2 deployment running for 7 seconds - 2/1 pods",
"#1 deployed 8 seconds ago",
"#1 deployed less than a second ago",
"#2 test deployment running for 7 seconds - 2/1 pods",
"#1 test deployed 8 seconds ago",
"* bc/ruby-sample-build is pushing to istag/origin-ruby-sample:latest that is using is/origin-ruby-sample, but that image stream does not exist.",
"* The image trigger for dc/frontend will have no effect because is/origin-ruby-sample does not exist",
"View details with 'oc describe <resource>/<name>' or list everything with 'oc get all'.",
},
Time: mustParseTime("2015-04-07T04:12:25Z"),
Expand Down
2 changes: 2 additions & 0 deletions pkg/generate/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ type DeploymentConfigRef struct {
Images []*ImageRef
Env Environment
Labels map[string]string
AsTest bool
}

// DeploymentConfig creates a deploymentConfig resource from the deployment configuration reference
Expand Down Expand Up @@ -337,6 +338,7 @@ func (r *DeploymentConfigRef) DeploymentConfig() (*deployapi.DeploymentConfig, e
},
Spec: deployapi.DeploymentConfigSpec{
Replicas: 1,
Test: r.AsTest,
Selector: selector,
Template: &kapi.PodTemplateSpec{
ObjectMeta: kapi.ObjectMeta{
Expand Down
14 changes: 11 additions & 3 deletions pkg/generate/app/cmd/describe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,18 @@ func describeBuildPipelineWithImage(out io.Writer, ref app.ComponentReference, p
}
}
if pipeline.Deployment != nil {
if len(pipeline.Deployment.Images) > 1 {
fmt.Fprintf(out, " * This image will be deployed as part of deployment config %q\n", pipeline.Deployment.Name)
if pipeline.Deployment.AsTest {
if len(pipeline.Deployment.Images) > 1 {
fmt.Fprintf(out, " * This image will be test deployed as part of deployment config %q\n", pipeline.Deployment.Name)
} else {
fmt.Fprintf(out, " * This image will be test deployed in deployment config %q\n", pipeline.Deployment.Name)
}
} else {
fmt.Fprintf(out, " * This image will be deployed in deployment config %q\n", pipeline.Deployment.Name)
if len(pipeline.Deployment.Images) > 1 {
fmt.Fprintf(out, " * This image will be deployed as part of deployment config %q\n", pipeline.Deployment.Name)
} else {
fmt.Fprintf(out, " * This image will be deployed in deployment config %q\n", pipeline.Deployment.Name)
}
}
}
if match != nil && match.Image != nil {
Expand Down
6 changes: 4 additions & 2 deletions pkg/generate/app/cmd/newapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ type AppConfig struct {
ExpectToBuild bool
BinaryBuild bool
AllowMissingImages bool
Deploy bool

Deploy bool
AsTestDeployment bool

SourceImage string
SourceImagePath string
Expand Down Expand Up @@ -681,7 +683,7 @@ func (c *AppConfig) buildPipelines(components app.ComponentReferences, environme
}
}
if c.Deploy {
if err := pipeline.NeedsDeployment(environment, c.Labels); err != nil {
if err := pipeline.NeedsDeployment(environment, c.Labels, c.AsTestDeployment); err != nil {
return nil, fmt.Errorf("can't set up a deployment for %q: %v", refInput, err)
}
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/generate/app/pipeline.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ type Pipeline struct {
}

// NeedsDeployment sets the pipeline for deployment.
func (p *Pipeline) NeedsDeployment(env Environment, labels map[string]string) error {
func (p *Pipeline) NeedsDeployment(env Environment, labels map[string]string, asTest bool) error {
if p.Deployment != nil {
return nil
}
Expand All @@ -191,6 +191,7 @@ func (p *Pipeline) NeedsDeployment(env Environment, labels map[string]string) er
},
Env: env,
Labels: labels,
AsTest: asTest,
}
return nil
}
Expand Down
3 changes: 3 additions & 0 deletions test/cmd/newapp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ os::cmd::expect_success_and_not_text 'oc new-app mysql --dry-run' "runs as the '
# trigger and output should say 5.6
os::cmd::expect_success_and_text 'oc new-app mysql -o yaml' 'mysql:5.6'
os::cmd::expect_success_and_text 'oc new-app mysql --dry-run' 'tag "5.6" for "mysql"'
# test deployments are created with the boolean flag and printed in the UI
os::cmd::expect_success_and_text 'oc new-app mysql --dry-run --as-test' 'This image will be test deployed'
os::cmd::expect_success_and_text 'oc new-app mysql -o yaml --as-test' 'test: true'

# docker strategy with repo that has no dockerfile
os::cmd::expect_failure_and_text 'oc new-app https://github.com/openshift/nodejs-ex --strategy=docker' 'No Dockerfile was found'
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/app-scenarios/new-project-deployed-app.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ items:
terminationMessagePath: /dev/termination-log
dnsPolicy: ClusterFirst
restartPolicy: Always
test: true
Copy link
Contributor

@0xmichalis 0xmichalis Feb 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure this doesn't interfere with other tests

Copy link
Contributor Author

@smarterclayton smarterclayton Feb 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't

triggers:
- type: ConfigChange
status:
Expand Down