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

Refactor create deployment and add --port flag #91113

Merged
merged 2 commits into from May 27, 2020
Merged

Refactor create deployment and add --port flag #91113

merged 2 commits into from May 27, 2020

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented May 14, 2020 •
edited

What type of PR is this?
/kind cleanup
/kind feature

What this PR does / why we need it:
This is coming from discussions from #87077 to add --port flag support for kubectl create deployment command.

Special notes for your reviewer:
/assign @sallyom @zhouya0

Does this PR introduce a user-facing change?:

Add --port flag to kubectl create deployment

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 14, 2020
@soltysh
Copy link
Contributor Author

soltysh commented May 14, 2020

/sig cli
/priority backlog

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 14, 2020
@soltysh soltysh mentioned this pull request May 14, 2020
Merged
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl labels May 14, 2020
sftim
sftim reviewed May 14, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Is there a test to validate the new command line option behaves as expected? (if there isn't, that might be OK - I'm asking for clarity so that the situation is clear to reviewers)

staging/src/k8s.io/kubectl/pkg/cmd/create/create_deployment.go Outdated
Port: -1,
PrintFlags: genericclioptions.NewPrintFlags("created").WithTypeSetter(scheme.Scheme),
IOStreams: ioStreams,
}
}

// NewCmdCreateDeployment is a macro command to create a new deployment.
// This command is better known to users as `kubectl create deployment`.
// Note that this command overlaps significantly with the `kubectl run` command.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit / aside: This line might be out of date now.

@soltysh
Copy link
Contributor Author

soltysh commented May 14, 2020

/hold
I'll refactor this on top of #90676

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2020
@zhouya0 zhouya0 mentioned this pull request May 15, 2020
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2020
@zhouya0 zhouya0 mentioned this pull request May 18, 2020
Merged
@sftim sftim mentioned this pull request May 18, 2020
Closed
@zhouya0 zhouya0 mentioned this pull request May 20, 2020
Closed
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2020
@sftim
Copy link
Contributor

sftim commented May 26, 2020

Those look like flakes to me
/retest

@soltysh
Copy link
Contributor Author

soltysh commented May 26, 2020

Those look like flakes to me
/retest

No worries, I was waiting for #91199 to land first :)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2020
@soltysh
Copy link
Contributor Author

soltysh commented May 26, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 26, 2020
@soltysh
Copy link
Contributor Author

soltysh commented May 26, 2020

/retest

1 similar comment
@soltysh
Copy link
Contributor Author

soltysh commented May 26, 2020

/retest

@zhouya0
Copy link
Contributor

zhouya0 commented May 26, 2020

Seems testing fail with this:

kubernetes/test/cmd/apps.sh

Line 200 in f097cee

kubectl create deployment test-nginx-apps --image=k8s.gcr.io/nginx:test-cmd --generator=deployment-basic/apps.v1

@soltysh
Copy link
Contributor Author

soltysh commented May 26, 2020

Good catch, I missed that bit. I accidentally dropped the --generator flag which is deprecated but still has to be there.

@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh

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

@zhouya0
Copy link
Contributor

zhouya0 commented May 26, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2020
@zhouya0
Copy link
Contributor

zhouya0 commented May 27, 2020

/retest

2 similar comments
@zhouya0
Copy link
Contributor

zhouya0 commented May 27, 2020

/retest

@zhouya0
Copy link
Contributor

zhouya0 commented May 27, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 7c3f706 into kubernetes:master May 27, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone May 27, 2020
@zhouya0 zhouya0 mentioned this pull request May 27, 2020
Merged
@soltysh soltysh deleted the create_deployment branch May 28, 2020 14:37
@github-actions github-actions bot mentioned this pull request Jun 2, 2020
Open
@Xercoy Xercoy mentioned this pull request Jul 20, 2020
Closed
@zhouya0 zhouya0 mentioned this pull request Sep 15, 2020
Closed
@davex98 davex98 mentioned this pull request Nov 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@dixudx dixudx Awaiting requested review from dixudx

@zhouya0 zhouya0 Awaiting requested review from zhouya0

Assignees

@sallyom sallyom

@zhouya0 zhouya0

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@soltysh @sftim @zhouya0 @k8s-ci-robot @sallyom