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

Apiregistration v1alpha1→v1beta1 #45247

Merged
merged 2 commits into from
May 17, 2017
Merged

Apiregistration v1alpha1→v1beta1 #45247

merged 2 commits into from
May 17, 2017

Conversation

mbohlool
Copy link
Contributor

@mbohlool mbohlool commented May 2, 2017 •
edited
Loading

Promoting apiregistration api from v1alpha1 to v1beta1.

API Registration is responsible for registering an API Group/Version with
another kubernetes like API server. The APIService holds information
about the other API server in APIServiceSpec type as well as general
TypeMeta and ObjectMeta. The APIServiceSpec type have the main
configuration needed to do the aggregation. Any request coming for
specified Group/Version will be directed to the service defined by
ServiceReference (on port 443) after validating the target using provided
CABundle or skipping validation if development flag InsecureSkipTLSVerify
is set. Priority is controlling the order of this API group in the overall
discovery document.
The return status is a set of conditions for this aggregation. Currently
there is only one condition named "Available", if true, it means the
api/server requests will be redirected to specified API server.

API Registration is now in beta.

@mbohlool mbohlool self-assigned this May 2, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 2, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 2, 2017
@mbohlool mbohlool force-pushed the c3 branch 3 times, most recently from 4cb46a4 to 0a0d1c4 Compare May 3, 2017 01:09
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2017
@mbohlool mbohlool assigned lavalamp and unassigned mbohlool May 3, 2017
@mbohlool mbohlool requested a review from lavalamp May 3, 2017 17:45
@mbohlool mbohlool changed the title [WIP] Apiregistration alpha1->beta2 Apiregistration alpha1->beta2 May 3, 2017
@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 3, 2017
@mbohlool mbohlool changed the title Apiregistration alpha1->beta2 Apiregistration alpha1->beta1 May 3, 2017
@mbohlool mbohlool changed the title Apiregistration alpha1->beta1 Apiregistration alpha1→beta1 May 3, 2017
@mbohlool mbohlool changed the title Apiregistration alpha1→beta1 Apiregistration v1alpha1→v1beta1 May 3, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2017
lavalamp
lavalamp reviewed May 8, 2017
View reviewed changes
Copy link
Member

@lavalamp lavalamp left a comment

Choose a reason for hiding this comment

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

No changes to the types themselves?

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1/doc.go Outdated
// +k8s:deepcopy-gen=package,register
// +k8s:conversion-gen=k8s.io/kubernetes/vendor/k8s.io/kube-aggregator/pkg/apis/apiregistration

// API Registration is responsible for registering an API `Group`/`Version` with
Copy link
Member

Choose a reason for hiding this comment

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

"Package v1beta1 contains the API Registration API, which is..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1/doc.go Outdated

// API Registration is responsible for registering an API `Group`/`Version` with
// another kubernetes like API server. The `APIService` holds information
// about the other API server in `APIServiceSpec` type as well as general
Copy link
Member

Choose a reason for hiding this comment

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

Suggest explaining the types in comments on the types instead of in the package comment?

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 was thinking a general idea of what these APIs are doing is helpful. I think I can sell this instead of one pager Brian asked for, what do you think?

cmd/libs/go2idl/go-to-protobuf/protobuf/cmd.go
@@ -68,7 +68,7 @@ func New() *Generator {
`+k8s.io/apimachinery/pkg/runtime`,
`k8s.io/apimachinery/pkg/apis/meta/v1`,
`k8s.io/apiserver/pkg/apis/example/v1`,
`k8s.io/kube-aggregator/pkg/apis/apiregistration/v1alpha1`,
`k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1`,
Copy link
Member

Choose a reason for hiding this comment

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

not a thing to change in this PR but I think mixing api group sources in the client generator default list like this is a bad precedent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

mbohlool
mbohlool commented May 9, 2017
View reviewed changes
Copy link
Contributor Author

@mbohlool mbohlool left a comment

Choose a reason for hiding this comment

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

Correct. No change to API types themselves. I looked at them and they look fine to me. Any suggestion?

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1/doc.go Outdated

// API Registration is responsible for registering an API `Group`/`Version` with
// another kubernetes like API server. The `APIService` holds information
// about the other API server in `APIServiceSpec` type as well as general
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 was thinking a general idea of what these APIs are doing is helpful. I think I can sell this instead of one pager Brian asked for, what do you think?

cmd/libs/go2idl/go-to-protobuf/protobuf/cmd.go
@@ -68,7 +68,7 @@ func New() *Generator {
`+k8s.io/apimachinery/pkg/runtime`,
`k8s.io/apimachinery/pkg/apis/meta/v1`,
`k8s.io/apiserver/pkg/apis/example/v1`,
`k8s.io/kube-aggregator/pkg/apis/apiregistration/v1alpha1`,
`k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1/doc.go Outdated
// +k8s:deepcopy-gen=package,register
// +k8s:conversion-gen=k8s.io/kubernetes/vendor/k8s.io/kube-aggregator/pkg/apis/apiregistration

// API Registration is responsible for registering an API `Group`/`Version` with
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2017
@lavalamp
Copy link
Member

@smarterclayton do you know why the proto thing changed all the binary blobs? I have seen this in a few places now.

@deads2k any objections to making this API beta?

@lavalamp
Copy link
Member

This will break the sample apiserver config, I guess. We can fix it up once this merges. (fyi @cheftako )

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2017
@deads2k
Copy link
Contributor

deads2k commented May 15, 2017

@deads2k any objections to making this API beta?

Sounds good to me.

@smarterclayton smarterclayton self-assigned this May 15, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2017
@smarterclayton
Copy link
Contributor

Proto change was because protobuf tar/gzs up the .proto into the generated.pb.go file (and I can't figure out how to disable it without invasive changes). Whenever anything in there changes, or if the Go version changes how gzip works, the contents change.

@smarterclayton
Copy link
Contributor

LGTM, @deads2k any other comments?

@mbohlool
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@deads2k
Copy link
Contributor

deads2k commented May 16, 2017

/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 16, 2017
@lavalamp
Copy link
Member

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, lavalamp, mbohlool

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45247, 45810, 45034, 45898, 45899)

@k8s-github-robot k8s-github-robot merged commit 3f0ebbe into kubernetes:master May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

Assignees

@lavalamp lavalamp

@smarterclayton smarterclayton

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mbohlool @k8s-reviewable @lavalamp @deads2k @smarterclayton @k8s-github-robot @k8s-ci-robot