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

Move /pkg/kubectl/cmd/{command} to staging #80876

Merged
merged 7 commits into from Aug 2, 2019
Merged

Move /pkg/kubectl/cmd/{command} to staging #80876

merged 7 commits into from Aug 2, 2019

Conversation

seans3
Copy link
Contributor

@seans3 seans3 commented Aug 1, 2019 •
edited

  • Moves pkg/kubectl/cmd/{command} to staging, except for auth, convert, and get which still have kubernetes dependencies.
  • This is a very large PR, moving almost half of the pkg/kubectl codebase to staging
  • Publishes this code to the new kubectl repository: https://github.com/kubernetes/kubectl/pkg/cmd/{command}.
  • Code will be available for re-use after publishing.

/kind cleanup
/sig cli
/area kubectl
/area code-organization
/priority important-soon

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/cli Categorizes an issue or PR as relevant to SIG CLI. area/kubectl size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/code-organization Issues or PRs related to kubernetes code organization priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 1, 2019
@seans3 seans3 changed the title Move kubectl cmd Move /pkg/kubectl/cmd/{command} to staging Aug 1, 2019
@k8s-ci-robot k8s-ci-robot added area/dependency Issues or PRs related to dependency changes sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Aug 1, 2019
@k8s-ci-robot k8s-ci-robot requested review from adohe-zz, apelisse and a team August 1, 2019 22:42
@seans3
Copy link
Contributor Author

seans3 commented Aug 1, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@seans3
Copy link
Contributor Author

seans3 commented Aug 1, 2019

/assign @liggitt
/cc @BenTheElder
/cc @dims
/cc @nikhita
/uncc @adohe

@k8s-ci-robot k8s-ci-robot requested review from BenTheElder, dims and nikhita and removed request for adohe-zz August 1, 2019 23:16
@BenTheElder
Copy link
Member

BenTheElder commented Aug 1, 2019 via email

@dims
Copy link
Member

dims commented Aug 1, 2019

/approve
/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2019
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 1, 2019
BenTheElder
BenTheElder approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2019
@seans3
Copy link
Contributor Author

seans3 commented Aug 2, 2019

/retest

liggitt
liggitt reviewed Aug 2, 2019
View reviewed changes
staging/src/k8s.io/kubectl/pkg/cmd/profiling.go
flags.StringVar(&profileName, "profile", "none", "Name of profile to capture. One of (none|cpu|heap|goroutine|threadcreate|block|mutex)")
flags.StringVar(&profileOutput, "profile-output", "profile.pprof", "Name of the file to write the profile to")
}

func initProfiling() error {
func InitProfiling() error {
Copy link
Member

@liggitt liggitt Aug 2, 2019 •
edited

Choose a reason for hiding this comment

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

not sure how I feel about exporting this modifying global package vars. would something like this be better?

type ProfileOptions struct {
  ProfileName   string
  ProfileOutput string
}
func (p *ProfileOptions) AddFlags(flags *pflag.FlagSet) {
  ... bind to p.ProfileName, p.ProfileOutput ...
}
func (p *ProfileOptions) Init() error {
  ... switch on p.ProfileName ...
}
func (p *ProfileOptions) Flush() error {
  ... switch on p.ProfileName, flush to p.ProfileOutput ...
}

Copy link
Member

Choose a reason for hiding this comment

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

and that options struct could be under k8s.io/kubectl/pkg/util/profiling

Copy link
Member

Choose a reason for hiding this comment

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

I'll take this as a follow up since this PR is a rebase magnet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll follow up on this in the next PR if that is OK.

liggitt
liggitt reviewed Aug 2, 2019
View reviewed changes
staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go
@@ -54,7 +54,7 @@ import (
)

var (
fakeSchema = sptest.Fake{Path: filepath.Join("..", "..", "..", "..", "api", "openapi-spec", "swagger.json")}
Copy link
Member

Choose a reason for hiding this comment

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

was fakeSchema pointing at the real static schema before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. There are several instances of this in the tests.

@liggitt
Copy link
Member

liggitt commented Aug 2, 2019

/lgtm
/approve

please open a follow up resolving #80876 (comment)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, dims, liggitt, seans3

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

nikhita
nikhita reviewed Aug 2, 2019
View reviewed changes
staging/publishing/rules.yaml
@@ -643,6 +645,8 @@ rules:
- repository: client-go
branch: release-12.0
- repository: code-generator
branch: release-1.15
- repository: component-base
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really need to be added to 1.15 anymore. It leads to an extra unneeded sync but is a no-op so I'll clean up redundant rules in a follow-up. Example - kubernetes/kubectl@f16387a

@k8s-ci-robot k8s-ci-robot merged commit 81f021b into kubernetes:master Aug 2, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 2, 2019
@nikhita nikhita mentioned this pull request Aug 2, 2019
Merged
@seans3 seans3 deleted the move-kubectl-cmd branch August 21, 2019 21:51
@liggitt liggitt removed the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 18, 2020
wking pushed a commit to wking/kubernetes that referenced this pull request Jul 21, 2020
@k8s-publishing-bot
Merge pull request kubernetes#80876 from seans3/move-kubectl-cmd
7b01e27 
Move /pkg/kubectl/cmd/{command} to staging

Kubernetes-commit: 81f021b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@nikhita nikhita nikhita left review comments

@BenTheElder BenTheElder BenTheElder approved these changes

@apelisse apelisse Awaiting requested review from apelisse

@dims dims Awaiting requested review from dims

Assignees

@dims dims

@BenTheElder BenTheElder

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-organization Issues or PRs related to kubernetes code organization area/dependency Issues or PRs related to dependency changes area/kubectl area/release-eng Issues or PRs related to the Release Engineering subproject 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.16
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@seans3 @BenTheElder @dims @liggitt @k8s-ci-robot @fejta-bot @nikhita