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

Replace underscores with dashes while rendering container names #606

Merged
merged 1 commit into from
May 17, 2017
Merged

Replace underscores with dashes while rendering container names #606

merged 1 commit into from
May 17, 2017

Conversation

achanda
Copy link
Contributor

@achanda achanda commented May 16, 2017

Kubernetes container names must match the regex a-z0-9?
This excludes underscores, which is common in container names in compose. Given this docker compose file

version: '2'
services:
  app:
    build: app
    container_name: forest_app
    image: app

Before this patch, compose generates this deployment

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  creationTimestamp: null
  name: app
spec:
  replicas: 1
  strategy: {}
  template:
    metadata:
      creationTimestamp: null
      labels:
        io.kompose.service: app
    spec:
      containers:
      - image: app
        name: forest_app
        resources: {}
      restartPolicy: Always
status: {}

After the patch, it generates

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  creationTimestamp: null
  name: app
spec:
  replicas: 1
  strategy: {}
  template:
    metadata:
      creationTimestamp: null
      labels:
        io.kompose.service: app
    spec:
      containers:
      - image: app
        name: forest-app
        resources: {}
      restartPolicy: Always
status: {}

The CLI output:

# ./kompose -f test.yaml convert
INFO Container name in docker-compose has been changed from "forest_app" to "forest-app"
WARN Kubernetes provider doesn't support build key - ignoring
INFO file "app-service.yaml" created
INFO file "app-deployment.yaml" created

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 16, 2017
@achanda
Copy link
Contributor Author

achanda commented May 16, 2017

Signed!

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 16, 2017
@cdrage
Copy link
Member

cdrage commented May 16, 2017

@achanda This looking great! Code LGTM 👍 💯

@achanda
Copy link
Contributor Author

achanda commented May 16, 2017

@cdrage I took the liberty of refactoring this so that we call the normalization code only once.

newName := normalizeServiceNames(composeServiceConfig.ContainerName)
serviceConfig.ContainerName = newName
if newName != composeServiceConfig.ContainerName {
log.Infof("Container name in docker-compose has been changed from %q to %q", composeServiceConfig.ContainerName, newName)
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we change the log to Container name in %q has been changed from %q to %q where the first %q being variable name this shows what service we have made change in!

@surajssd
Copy link
Member

@achanda thanks for the PR, apart from that nit code LGTM!

Kubernetes container names must match the regex [a-z0-9]([-a-z0-9]*[a-z0-9])?
This excludes underscores, which is common in container names in compose.
@achanda
Copy link
Contributor Author

achanda commented May 17, 2017

@surajssd thanks, done!

@surajnarwade
Copy link
Contributor

@achanda LGTM

@surajssd
Copy link
Member

@achanda awesome!

@surajssd surajssd merged commit 22fe599 into kubernetes:master May 17, 2017
@achanda achanda deleted the underscores branch May 17, 2017 12:48
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.

5 participants