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

add APIService conditions #43301

Merged
merged 2 commits into from
Apr 29, 2017
Merged

add APIService conditions #43301

merged 2 commits into from
Apr 29, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 17, 2017

Adds conditions to the APIServiceStatus struct and fixes up generators that appear to have slipped.

The first condition is "ServiceAvailable" which will provide the status currently derived in the discovery handler that decides about whether to expose the version in discovery.

@kubernetes/sig-api-machinery-pr-reviews @liggitt @ncdc

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k
We suggest the following additional approver: @bgrant0607

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-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Mar 17, 2017
@deads2k deads2k added this to the v1.7 milestone Mar 17, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@deads2k deads2k force-pushed the agg-27-add-conditions branch 3 times, most recently from 0f2f645 to c45e3df Compare March 20, 2017 13:23
@deads2k
Copy link
Contributor Author

deads2k commented Mar 20, 2017 

@k8s-bot gce etcd3 e2e test this

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2017
@luxas
Copy link
Member

luxas commented Apr 3, 2017

@deads2k will you rebase this one anytime soon?
I'd love to have this working at HEAD ;)

deads2k
deads2k commented Apr 10, 2017
View reviewed changes
staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go Outdated
type APIServiceConditionType string

const (
// ServiceAvailable indicates that the service exists and has endpoints
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given other changes: "indicates that the service exists and is reachable"

@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2017

@liggitt @lavalamp second commit has alpha API changes for status.

liggitt
liggitt reviewed Apr 12, 2017
View reviewed changes
staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go
const (
ConditionTrue ConditionStatus = "True"
ConditionFalse ConditionStatus = "False"
ConditionUnknown ConditionStatus = "Unknown"
Copy link
Member

Choose a reason for hiding this comment

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

Are missing conditions treated as Unknown, or is that decided per condition type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are missing conditions treated as Unknown, or is that decided per condition type?

There is no guarantee that every condition will be present and there is no defaulting on these conditions.. How a client uses the data is up to them. There is a difference between "Unknown" and <missing>, so hopefully clients know which one they want to test for.

@liggitt
Copy link
Member

liggitt commented Apr 12, 2017

assuming this is mostly replicating conditions from other API objects

@deads2k
Copy link
Contributor Author

deads2k commented Apr 12, 2017

assuming this is mostly replicating conditions from other API objects

correct.

@deads2k deads2k force-pushed the agg-27-add-conditions branch from c45e3df to 4a97b3e Compare April 12, 2017 20:30
@deads2k
Copy link
Contributor Author

deads2k commented Apr 12, 2017

Comments addressed.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 12, 2017
@deads2k deads2k force-pushed the agg-27-add-conditions branch from 4a97b3e to df3fdfa Compare April 13, 2017 14:30
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2017
@deads2k deads2k force-pushed the agg-27-add-conditions branch from df3fdfa to a3c9ec6 Compare April 13, 2017 15:35
@deads2k
Copy link
Contributor Author

deads2k commented Apr 13, 2017

@k8s-bot cvm gce e2e test this

@luxas luxas added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 16, 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 Apr 16, 2017
@luxas
Copy link
Member

luxas commented Apr 16, 2017

ping @liggitt @ncdc
It would be great to get this in ASAP :)

liggitt
liggitt reviewed Apr 25, 2017
View reviewed changes
staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go
type APIServiceConditionType string

const (
// Available indicates that the service exists and is reachable
Copy link
Member

Choose a reason for hiding this comment

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

does this single condition type encompass indicating the resource/group/version was not claimed by another API service, and that the backend service was reachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this single condition type encompass indicating the resource/group/version was not claimed by another API service, and that the backend service was reachable?

Request from @smarterclayton. I think it lacks granularity, but I need something more than I need my preferred thing and as a summary it's an ok name.

@liggitt
Copy link
Member

liggitt commented Apr 25, 2017 •
edited
Loading

one question, no other comments

@deads2k deads2k force-pushed the agg-27-add-conditions branch from a3c9ec6 to afc5ae1 Compare April 28, 2017 15:28
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2017
@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Apr 28, 2017

top level stuff is straight generated. approving.

@deads2k deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2017
@luxas
Copy link
Member

luxas commented Apr 28, 2017

LGTM, thank you @deads2k!

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 44044, 44766, 44930, 45109, 43301)

@k8s-github-robot k8s-github-robot merged commit ce01882 into kubernetes:master Apr 29, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 29, 2017 •
edited
Loading

@deads2k: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins Bazel Build afc5ae1 link @k8s-bot bazel test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@luxas luxas mentioned this pull request Apr 29, 2017
Closed
@deads2k deads2k deleted the agg-27-add-conditions branch August 3, 2017 20:08
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

Assignees

@ncdc ncdc

@liggitt liggitt

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
v1.7
Development

Successfully merging this pull request may close these issues.
7 participants
@deads2k @k8s-github-robot @k8s-reviewable @luxas @liggitt @k8s-ci-robot @ncdc