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

create service add create ExternalName service implementation #34789

Merged
merged 1 commit into from
Dec 2, 2016
Merged

create service add create ExternalName service implementation #34789

merged 1 commit into from
Dec 2, 2016

Conversation

adohe-zz
Copy link

@adohe-zz adohe-zz commented Oct 14, 2016 •
edited by soltysh
Loading

@kubernetes/kubectl create service add ExternalName support, refer #34731 for more detail.

kubectl create service externalname

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 14, 2016
@adohe-zz adohe-zz changed the title create service add create ExternalName service implementation [WIP] create service add create ExternalName service implementation Oct 14, 2016
@deads2k deads2k assigned fabianofranz and unassigned deads2k Oct 14, 2016
@smarterclayton
Copy link
Contributor

Can you add a test to test-cmd?

fabianofranz
fabianofranz reviewed Oct 14, 2016
View reviewed changes
pkg/kubectl/cmd/create_service.go
)

// NewCmdCreateServiceExternalName is a macro command for creating a ExternalName service
func NewCmdCreteServiceExternalName(f cmdutil.Factory, cmdOut io.Writer) *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

NewCmdCreateServiceExternalName

Copy link
Author

Choose a reason for hiding this comment

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

stupid error :(

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2016
@adohe-zz adohe-zz changed the title [WIP] create service add create ExternalName service implementation create service add create ExternalName service implementation Oct 19, 2016
@adohe-zz
Copy link
Author

tests add, comment addressed @smarterclayton @fabianofranz ptal.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/old-docs needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 19, 2016
@adohe-zz
Copy link
Author

@fabianofranz @smarterclayton rebased, ptal.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 8780e55a43d0397feac1969d983097cc3edb7fbd. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 8780e55a43d0397feac1969d983097cc3edb7fbd. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 8780e55a43d0397feac1969d983097cc3edb7fbd. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 8780e55a43d0397feac1969d983097cc3edb7fbd. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 8780e55a43d0397feac1969d983097cc3edb7fbd. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 8780e55a43d0397feac1969d983097cc3edb7fbd. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 8780e55a43d0397feac1969d983097cc3edb7fbd. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/old-docs labels Oct 22, 2016
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 10, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit c10c6835ce4c96304fb5bfaf784e4a389e80fcd7. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@apelisse apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@adohe-zz
Copy link
Author

@fabianofranz I just go back to this, please help review this :)

fabianofranz
fabianofranz reviewed Nov 14, 2016
View reviewed changes
Copy link
Contributor

@fabianofranz fabianofranz left a comment

Choose a reason for hiding this comment

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

Just minor nits otherwise LGTM. Just note that this might be out of 1.5 feature freeze.

pkg/kubectl/cmd/create_service.go
cmdutil.AddPrinterFlags(cmd)
cmdutil.AddGeneratorFlags(cmd, cmdutil.ServiceExternalNameGeneratorV1Name)
addPortFlags(cmd)
cmd.Flags().String("external-name", "", "external name of service")
Copy link
Contributor

Choose a reason for hiding this comment

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

--external-name is required, correct? In which case it needs cmd.MarkFlagRequired for completions.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

pkg/kubectl/cmd/create_service.go

var (
serviceExternalNameLong = templates.LongDesc(`
Create an ExternalName service with the specified name.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to mention here what ExternalName services can be used for? Like "can be used for CNAME", etc. This may not be very clear for used in a first sight.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

pkg/kubectl/cmd/create_service.go
Create an ExternalName service with the specified name.`)

serviceExternalNameExample = templates.Examples(`
# Create a new ExternalName service named bar.com
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact it is named "my-ns", "bar.com" being the external name.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@adohe-zz
Copy link
Author

@fabianofranz update, please take a look.

@adohe-zz
Copy link
Author

@fabianofranz That's ok if we cannot include this in 1.5

@fabianofranz
Copy link
Contributor

@adohe needs a release note since it introduces a new command. ;)

@adohe-zz
Copy link
Author

Release note:

kubectl create service add create ExternalName service support

@fabianofranz ptal

@fabianofranz
Copy link
Contributor

LGTM, waiting for post-1.5.

@soltysh soltysh added this to the v1.6 milestone Nov 21, 2016
@soltysh
Copy link
Contributor

soltysh commented Nov 21, 2016

Applying 1.6 and lgtm labels per Fabiano's comment.

@soltysh soltysh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2016
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 21, 2016
@adohe-zz
Copy link
Author

@soltysh Could you tell me how to add some release note here? I already add some, but seems bot didn't notice this.

@0xmichalis 0xmichalis added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Nov 21, 2016
@smarterclayton
Copy link
Contributor

smarterclayton commented Nov 21, 2016 •
edited
Loading

You need to do:

```release-note
```

in the PR description.

@adohe-zz
Copy link
Author

@smarterclayton I think @soltysh already helped add this, thanks all the same. :)

@fabianofranz
Copy link
Contributor

@k8s-bot test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit bbd447f into kubernetes:master Dec 2, 2016
@adohe-zz adohe-zz deleted the external_service branch December 2, 2016 15:32
@0xmichalis 0xmichalis mentioned this pull request Jan 16, 2017
Closed
@therc therc mentioned this pull request Jan 24, 2017
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianofranz fabianofranz fabianofranz left review comments

Assignees

@fabianofranz fabianofranz

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet