Skip to content
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

Added support for different namespaces #517

Merged
merged 1 commit into from
Apr 12, 2017

Conversation

procrypt
Copy link
Contributor

Now we can deploy application in different namespaces using the --namespace=<value> flag with kompose up and kompose down. The --namepace flag will deploy the application in that particular namespace if, exist.

docker-compose file used.

version: "2"
services:
  web:
    image: tuna/docker-counter23:latest
    ports:
      - "5000:5000"
    links:
      - redis
  redis:
    image: redis:latest
    ports:
      - "6379"

Current namespaces in k8s cluster

$ kubectl get namespace 
NAME          STATUS    AGE
default       Active    3m
development   Active    3m
kube-system   Active    3m

Deploy application in namespace=development

$ kompose up --namespace=development
We are going to create Kubernetes Deployments, Services and PersistentVolumeClaims for your Dockerized application. 
If you need different kind of resources, use the 'kompose convert' and 'kubectl create -f' commands instead. 

INFO[0000] Deploying application in "development" namespace
 
INFO[0000] Successfully created Service: redis          
INFO[0000] Successfully created Service: web            
INFO[0000] Successfully created Deployment: redis       
INFO[0000] Successfully created Deployment: web         

Your application has been deployed to Kubernetes. You can run 'kubectl get deployment,svc,pods,pvc' for details.

Verify application been deployed in the namespace=development correctly

$ kubectl get deployment,svc,pods,pvc --namespace=development
NAME                        DESIRED      CURRENT             UP-TO-DATE   AVAILABLE   AGE
redis                       1            1                   1            0           2m
web                         1            1                   1            0           2m
NAME                        CLUSTER-IP   EXTERNAL-IP         PORT(S)      AGE
redis                       10.0.0.110   <none>              6379/TCP     2m
web                         10.0.0.55    <none>              5000/TCP     2m
NAME                        READY        STATUS              RESTARTS     AGE
mlbparks-3930800586-mblgc   0/1          Terminating         0            4m
redis-741327525-om0xc       0/1          ContainerCreating   0            2m
web-519030457-28arz         0/1          ContainerCreating   0            2m

Delete the application that has been deployed in the namespace=development

$ kompose down --namespace=development
INFO[0000] Deleting application from "development" namespace
 
INFO[0000] Successfully deleted Service: redis          
INFO[0000] Successfully deleted Service: web            
INFO[0003] Successfully deleted Deployment: redis       
INFO[0006] Successfully deleted Deployment: web  

Fixes: #473

CC: @kadel @cdrage @containscafeine @surajssd

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 27, 2017
@surajssd
Copy link
Member

if this is being done for up and down then this can/should also be done for convert ??

@procrypt
Copy link
Contributor Author

procrypt commented Mar 28, 2017

@surajssd How does it make sense to add this to convert? We can deploy the artifacts created by kompose convert in any namespace we want using kubectl for example,

$ kubectl create -f <file> --namespace=development

Please CMIIW.

@cdrage
Copy link
Member

cdrage commented Mar 28, 2017

I agree with @procrypt that this doesn't make sense to add to convert. Convert simply converts the files and outputs it in the same directory. Using kubectl to bring them up.

cmd/down.go Outdated
@@ -56,5 +58,6 @@ var downCmd = &cobra.Command{
func init() {
downCmd.Flags().BoolVar(&DownEmptyVols, "emptyvols", false, "Use Empty Volumes. Do not generate PVCs")
downCmd.Flags().IntVar(&DownReplicas, "replicas", 1, "Specify the number of repliaces in the generate resource spec")
downCmd.Flags().StringVar(&DownNamespace, "namespace", "", " Specify Namespace to deploy your application")
Copy link
Member

Choose a reason for hiding this comment

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

There's a space before Specify

It may be good to add a default value to this as well, using default namespace.

cmd/up.go Outdated
@@ -56,5 +58,6 @@ var upCmd = &cobra.Command{
func init() {
upCmd.Flags().BoolVar(&UpEmptyVols, "emptyvols", false, "Use Empty Volumes. Do not generate PVCs")
upCmd.Flags().IntVar(&UpReplicas, "replicas", 1, "Specify the number of repliaces in the generate resource spec")
upCmd.Flags().StringVar(&UpNamespace, "namespace", "", " Specify Namespace to deploy your application")
Copy link
Member

Choose a reason for hiding this comment

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

space before Specify

client, namespace, err := k.GetKubernetesClient()
client, ns, err := k.GetKubernetesClient()
namespace := opt.Namespace
if len(namespace) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

In In order to avoid all of this len and checking against 0, have a look at how we check if a default value is changed in convert.go.

For example:

      IsDeploymentFlag:            cmd.Flags().Lookup("deployment").Changed,

on line 73.

Instead, you can check to see if the --namespace flag has been set via:

      IsNamespaceFlag:            cmd.Flags().Lookup("namespace").Changed,

and simply check with

if opt.IsNamespaceFlag {
...
}

in the resulting code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdrage Thanks for pointing it out. I have updated the PR 😉

@procrypt procrypt force-pushed the new_namespace branch 2 times, most recently from 5c4bc56 to 7d5264a Compare April 3, 2017 09:14
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Other than a small nitpick, this LGTM.

if err != nil {
return err
}
log.Infof("Deploying application in %q namespace\n", namespace)
Copy link
Member

Choose a reason for hiding this comment

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

No \n. Would just create unneeded space in the output.

@procrypt procrypt force-pushed the new_namespace branch 2 times, most recently from 847c82d to 5d8489e Compare April 3, 2017 19:14
@procrypt
Copy link
Contributor Author

procrypt commented Apr 3, 2017

@cdrage Updated PR

@cdrage
Copy link
Member

cdrage commented Apr 3, 2017

LGTM!

Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

We have to add this also for OpenShift

if opt.IsNamespaceFlag {
namespace = opt.Namespace
}

Copy link
Member

Choose a reason for hiding this comment

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

I would add similar message as in up informing what namespace used.

Copy link
Contributor Author

@procrypt procrypt Apr 5, 2017

Choose a reason for hiding this comment

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

@kadel I had added it for down earlier but the issue I faced was, if we run kompose down and there is nothing deployed kompose will still show

INFO[0000] Deleting application in "default" namespace

Copy link
Member

Choose a reason for hiding this comment

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

Hey @procrypt
You may need a rebase since Kompose shouldn't be outputting [0000] in the timestamp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdrage Rebased and updated.

@procrypt procrypt force-pushed the new_namespace branch 7 times, most recently from 5154e1d to 4764c21 Compare April 10, 2017 07:24
@procrypt
Copy link
Contributor Author

Ping @kadel @cdrage

@procrypt procrypt closed this Apr 11, 2017
@procrypt procrypt reopened this Apr 11, 2017
@procrypt procrypt force-pushed the new_namespace branch 3 times, most recently from 2b5f4d0 to 3ae8a58 Compare April 11, 2017 12:17
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

one space, and up.go is not formatted according to gofmt, otherwise it looks good

cmd/up.go Outdated
@@ -59,5 +62,6 @@ func init() {
upCmd.Flags().BoolVar(&UpEmptyVols, "emptyvols", false, "Use empty volumes. Do not generate PersistentVolumeClaim")
upCmd.Flags().IntVar(&UpReplicas, "replicas", 1, "Specify the number of replicas generated")
upCmd.Flags().BoolVar(&UpInsecureRepo, "insecure-repository", false, "Use an insecure Docker repository for OpenShift ImageStream")
upCmd.Flags().StringVar(&UpNamespace, "namespace", "default", " Specify Namespace to deploy your application")
Copy link
Member

Choose a reason for hiding this comment

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

There is a space before "Specify"

@procrypt procrypt force-pushed the new_namespace branch 4 times, most recently from a5badc0 to 0c1a115 Compare April 12, 2017 13:09
Now we can deploy application in different namespaces using the "--namespace=<value>" flag with kompose up and kompose down. The --namepace flag will deploy the application in that particular "namespace" if exist."
@procrypt
Copy link
Contributor Author

@kadel @cdrage All green now ☺️

@kadel kadel merged commit 3dd41ec into kubernetes:master Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants