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

Create the k8s.io/component-base staging repo #72569

Merged
merged 6 commits into from Jan 8, 2019

Conversation

@luxas
Copy link
Member

luxas commented Jan 4, 2019

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Implements first part of KEP https://github.com/kubernetes/enhancements/blob/master/keps/sig-cluster-lifecycle/0032-create-a-k8s-io-component-repo.md

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Co-authored-by: @Klaven

Does this PR introduce a user-facing change?:

NONE

/assign @sttts @mtaufen
cc @kubernetes/sig-cluster-lifecycle-pr-reviews

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Jan 4, 2019

/priority important-longterm

@Klaven
Copy link
Contributor

Klaven left a comment

Looks good to me, I have one question and a catch on an import name! Thanks you!

"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
apiserver "k8s.io/apiserver/pkg/apis/config/validation"
componentbase "k8s.io/component-base/config/validation"

This comment has been minimized.

@Klaven

Klaven Jan 4, 2019

Contributor

should this be componentbaseconfigvalidation? i'm not quite sure what the rules the community uses but is how many of the objects are imported.

This comment has been minimized.

@luxas

luxas Jan 4, 2019

Author Member

Good catch!

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apimachineryconfigv1alpha1 "k8s.io/component-base/config/v1alpha1"

This comment has been minimized.

@Klaven

Klaven Jan 4, 2019

Contributor

it appears that while the import path was updated, the name was left the same. This should be componentbaseconfigv1alpha1 "k8s.io/component-base/config/v1alpha1"

This comment has been minimized.

@luxas

luxas Jan 4, 2019

Author Member

Good catch!

@jbeda

This comment has been minimized.

Copy link
Contributor

jbeda commented Jan 4, 2019

/approve

- sttts
- mtaufen
- DirectXMan12
- liggitt

This comment has been minimized.

@sttts

sttts Jan 4, 2019

Contributor

please compare with KEP, I think the list differs.

This comment has been minimized.

@luxas

luxas Jan 4, 2019

Author Member

good catch, will do


### OWNERS

WG Component Standard is working on this refactoring process, which is happening incrementally, starting in the v1.14 cycle.

This comment has been minimized.

@sttts

sttts Jan 4, 2019

Contributor

is the WG a thing yet?

This comment has been minimized.

@luxas

luxas Jan 4, 2019

Author Member

It's approved enough I think to be ref'd here. Just some small-ish bikeshedding left at this stage :)

This comment has been minimized.

@mtaufen

mtaufen Jan 7, 2019

Contributor

Would it be better to just have the SIGs (API Machinery and Cluster Lifecycle) here, as the WG structure hasn't been finalized yet? We can add it later once that discussion is over, or if we just end up as a subproject after all.

@@ -87,9 +87,9 @@
"k8s.io/api/settings/v1alpha1",
"k8s.io/api/admission/v1beta1",
"k8s.io/api/networking/v1",
"k8s.io/api/admissionregistration/v1alpha1"
"k8s.io/api/admissionregistration/v1alpha1",

This comment has been minimized.

@sttts

sttts Jan 4, 2019

Contributor

please squash into correct commits

This comment has been minimized.

@luxas

luxas Jan 4, 2019

Author Member

will do

@@ -1,9 +1,6 @@
## component-base

**WARNING:** This staging repo is still in the experimental stage.

This comment has been minimized.

@sttts

sttts Jan 4, 2019

Contributor

I would leave this

This comment has been minimized.

@luxas

luxas Jan 4, 2019

Author Member

It's just said in an other place now

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Jan 4, 2019

I didn't see any real API changes, just moving stuff around - did I miss it?

@thockin exactly right. Only moving k8s.io/{apiserver,apimachinery}/pkg/apis/config into k8s.io/component-base/config now that we have real place for this.

I'll fix @sttts comments and squash to the final form. Thanks everybody for the reviews!


Do not open pull requests directly against this repository, they will be ignored. Instead, please open pull requests against [kubernetes/kubernetes](https://git.k8s.io/kubernetes/). Please follow the same [contributing guide](https://git.k8s.io/kubernetes/CONTRIBUTING.md) you would follow for any other pull request made to kubernetes/kubernetes.

This repository is published from [kubernetes/kubernetes/staging/src/k8s.io/api](https://git.k8s.io/kubernetes/staging/src/k8s.io/api) by the [kubernetes publishing-bot](https://git.k8s.io/publishing-bot).

This comment has been minimized.

@nikhita

nikhita Jan 4, 2019

Member

This needs to be component-base :)

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jan 4, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 4, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbeda, luxas, sttts, thockin

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

@@ -143,16 +143,19 @@
allowedImports:
- k8s.io/apimachinery
- k8s.io/apiserver
- k8s.io/component-base

This comment has been minimized.

@nikhita

nikhita Jan 4, 2019

Member

Can you also add component-base as a baseImportPath and then add apimachinery to the list of allowedImports?

@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Jan 4, 2019

One small nit: can you add component-base to the list of staging repos in the README here: https://github.com/kubernetes/kubernetes/tree/master/staging? 😬

@nikhita nikhita referenced this pull request Jan 4, 2019

Closed

Add component-base repo #142

4 of 4 tasks complete

luxas added some commits Jan 6, 2019

@luxas luxas force-pushed the luxas:component_base_init branch from 66d879c to d26181f Jan 6, 2019


## Purpose

Implement KEP 32: https://github.com/kubernetes/enhancements/blob/master/keps/sig-cluster-lifecycle/0032-create-a-k8s-io-component-repo.md

This comment has been minimized.

@dixudx

dixudx Jan 6, 2019

Member

Use [KEP 32: Create a k8s.io/component-base repo](https://github.com/kubernetes/enhancements/blob/master/keps/sig-cluster-lifecycle/0032-create-a-k8s-io-component-repo.md) instead?

This comment has been minimized.

@luxas

luxas Jan 6, 2019

Author Member

may improve in a later PR

@@ -50,6 +50,12 @@
- k8s.io/kube-openapi
- k8s.io/klog

- baseImportPath: "./vendor/k8s.io/component-base/"
allowedImports:
- k8s.io/apimachinery

This comment has been minimized.

@dixudx

dixudx Jan 6, 2019

Member

Will k8s.io/apimachinery import packages from k8s.io/component-base in the future? I think the answer is yes. Correct me if I am wrong. To avoid import circle, we'd better add a TODO or create an issue to address this after the whole refactoring.

This comment has been minimized.

@luxas

luxas Jan 6, 2019

Author Member

No, component-base will always depend on apimachinery.

luxas added some commits Jan 6, 2019

@luxas luxas force-pushed the luxas:component_base_init branch from d26181f to 1edd272 Jan 6, 2019

@@ -28,8 +28,7 @@ import (
"strings"
"time"

"k8s.io/api/core/v1"
apimachineryconfig "k8s.io/apimachinery/pkg/apis/config"
v1 "k8s.io/api/core/v1"

This comment has been minimized.

@stewart-yu

stewart-yu Jan 7, 2019

Contributor

no need rename?

This comment has been minimized.

@luxas

luxas Jan 8, 2019

Author Member

tbh I don't know from where this came, but it doesn't harm much 😄


- baseImportPath: "./vendor/k8s.io/kube-scheduler/"
allowedImports:
- k8s.io/apimachinery
- k8s.io/apiserver
- k8s.io/component-base
- k8s.io/klog

This comment has been minimized.

@stewart-yu

stewart-yu Jan 7, 2019

Contributor

After using k8s.io/component-base, can we drop both k8s.io/apimachinery and k8s.io/apiserver in allowedImports list?
and in others place?
may be a big work, should be a follow-up PR

This comment has been minimized.

@luxas

luxas Jan 8, 2019

Author Member

I'm not really sure, I think that you need to specify ALL imported k8s.io staging repos, and hence (as component-base depends on apimachinery) it'd be required anyway

This comment has been minimized.

@sttts

sttts Jan 8, 2019

Contributor

afaik there is no transitive closure computation. Feel free to add it.

- k8s.io/utils

- baseImportPath: "./vendor/k8s.io/kube-proxy/"
allowedImports:
- k8s.io/apimachinery
- k8s.io/component-base
- k8s.io/klog
- k8s.io/utils

This comment has been minimized.

@sttts

sttts Jan 8, 2019

Contributor

are these non-staging repos necessary here? As it was working before, I would gues the answer is no.

This comment has been minimized.

@luxas

luxas Jan 8, 2019

Author Member

I don't think they technically are necessary, but they were there for a couple of other projects IIRC so I was just consistent. We might wanna require all k8s.io projects to be listed here eventually?

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jan 8, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 8, 2019

@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Jan 8, 2019

@luxas

This comment has been minimized.

Copy link
Member Author

luxas commented Jan 8, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 1b28775 into kubernetes:master Jan 8, 2019

19 checks passed

cla/linuxfoundation luxas authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@luxas luxas added this to In progress in component-base via automation Jan 17, 2019

@luxas luxas moved this from In progress to Done in component-base Jan 18, 2019

@Klaven Klaven referenced this pull request Feb 7, 2019

Closed

REQUEST: New membership for @Klaven (Marek Counts) #455

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment