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.

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

Jump to bottom

Propagate dots into valid service names #1063

Merged
merged 8 commits into from
Sep 28, 2018
Merged

Propagate dots into valid service names #1063

merged 8 commits into from
Sep 28, 2018

Conversation

rcmorano
Copy link
Contributor

Fix approach for #1062; a similar bug than #420.

@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://git.k8s.io/community/CLA.md#the-contributor-license-agreement 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.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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 cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 11, 2018
@rcmorano rcmorano mentioned this pull request Aug 11, 2018
Closed
@rcmorano rcmorano force-pushed the fix-dot-in-service-names branch from 95c55d1 to 4bd497f Compare August 11, 2018 13:24
@rcmorano rcmorano force-pushed the fix-dot-in-service-names branch from 4bd497f to 0753f4e Compare August 11, 2018 13:25
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 11, 2018
@rcmorano
splitted normalization for volumes and service names into different f…
d67ccd3 
…unctions (it conflicted with './' volumes replacement)
@rcmorano rcmorano force-pushed the fix-dot-in-service-names branch 2 times, most recently from 6c50da8 to 3bc96b6 Compare August 11, 2018 14:44
@rcmorano rcmorano force-pushed the fix-dot-in-service-names branch 3 times, most recently from 1d1e13f to 979aa50 Compare August 11, 2018 16:09
@rcmorano
Copy link
Contributor Author

/check-cla

@rcmorano rcmorano force-pushed the fix-dot-in-service-names branch 3 times, most recently from e659114 to 5dfa8ee Compare August 13, 2018 07:22
@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 Aug 13, 2018
@rcmorano rcmorano force-pushed the fix-dot-in-service-names branch from 5dfa8ee to dd53429 Compare August 13, 2018 08:13
@hangyan hangyan self-assigned this Aug 15, 2018
hangyan
hangyan reviewed Aug 15, 2018
View reviewed changes
docs/user-guide.md Outdated
@@ -429,7 +429,7 @@ services:

If the Docker Compose file has a volume specified for a service, the Deployment (Kubernetes) or DeploymentConfig (OpenShift) strategy is changed to "Recreate" instead of "RollingUpdate" (default). This is done to avoid multiple instances of a service from accessing a volume at the same time.

If the Docker Compose file has service name with `_` in it (eg.`web_service`), then it will be replaced by `-` and the service name will be renamed accordingly (eg.`web-service`). Kompose does this because "Kubernetes" doesn't allow `_` in object name.
If the Docker Compose file has service name with `_` or `.` in it (eg.`web_service` or `web.service`), then it will be replaced by `-` and the service name will be renamed accordingly (eg.`web-service`). Kompose does this because "Kubernetes" doesn't allow `_` in object name nor metadata name (rises "DNS-1035 label must consist of lower case alphanumeric characters").
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't object name and metadata name the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Others LGTM

Copy link
Contributor Author

@rcmorano rcmorano Aug 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Right!

I made the distinction because the original documentation refered to a problem with the "spec" part of the kube file, but at the end, metadata is just another Kubernetes object so I guess it might be simplified to "object names" (plural :).

Copy link
Contributor

Choose a reason for hiding this comment

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

@rcmorano Sorry about the late response, update the doc and we are ready to merge!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: hangyan

If they are not already assigned, you can assign the PR to them by writing /assign @hangyan in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rcmorano
Copy link
Contributor Author

/assign @hangyan

hangyan
hangyan approved these changes Sep 28, 2018
View reviewed changes
Copy link
Contributor

@hangyan hangyan left a comment

Choose a reason for hiding this comment

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

LGTM

docs/user-guide.md Outdated
@@ -429,7 +429,7 @@ services:

If the Docker Compose file has a volume specified for a service, the Deployment (Kubernetes) or DeploymentConfig (OpenShift) strategy is changed to "Recreate" instead of "RollingUpdate" (default). This is done to avoid multiple instances of a service from accessing a volume at the same time.

If the Docker Compose file has service name with `_` in it (eg.`web_service`), then it will be replaced by `-` and the service name will be renamed accordingly (eg.`web-service`). Kompose does this because "Kubernetes" doesn't allow `_` in object name.
If the Docker Compose file has service name with `_` or `.` in it (eg.`web_service` or `web.service`), then it will be replaced by `-` and the service name will be renamed accordingly (eg.`web-service`). Kompose does this because "Kubernetes" doesn't allow `_` in object name nor metadata name (rises "DNS-1035 label must consist of lower case alphanumeric characters").
Copy link
Contributor

Choose a reason for hiding this comment

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

@rcmorano Sorry about the late response, update the doc and we are ready to merge!

@hangyan hangyan merged commit 424f1a9 into kubernetes:master Sep 28, 2018
@hangyan
Copy link
Contributor

hangyan commented Sep 28, 2018

@rcmorano Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangyan hangyan hangyan approved these changes

Assignees

@hangyan hangyan

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rcmorano @k8s-ci-robot @hangyan