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

CRD Status subresource support get+update+patch #63619

Merged
merged 2 commits into from
May 11, 2018
Merged

CRD Status subresource support get+update+patch #63619

merged 2 commits into from
May 11, 2018

Conversation

roycaihw
Copy link
Member

@roycaihw roycaihw commented May 9, 2018 •
edited by sttts
Loading

CRD Status previously only supports PUT and returns 405 on GET and PATCH

/assign @sttts
/sig api-machinery

Release note:

CustomResourceDefinitions Status subresource now supports GET and PATCH

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 9, 2018
@sttts
Copy link
Contributor

sttts commented May 9, 2018

We need a test for that in test/integration in the apiextensions-apiserver.

@sttts
Copy link
Contributor

sttts commented May 9, 2018

/assign @nikhita

liggitt
liggitt reviewed May 9, 2018
View reviewed changes
staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go
@@ -160,12 +160,17 @@ type StatusREST struct {
store *genericregistry.Store
}

var _ = rest.Updater(&StatusREST{})
var _ = rest.Patcher(&StatusREST{})
Copy link
Member

Choose a reason for hiding this comment

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

nit: keep the Updater assertion and add Patcher

Copy link
Member Author

Choose a reason for hiding this comment

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

Patcher assertion implies Updater assertion

kubernetes/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go

Lines 252 to 256 in c94efce

// Patcher is a storage object that supports both get and update.
type Patcher interface {
Getter
Updater
}

but we don't have a lot of Updater/Patcher assertion in our code base (grep result is 1 Updater assertion and 6 Patcher assertion), so I don't know what is good convention/style

Copy link
Member

Choose a reason for hiding this comment

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

This is fine as is, then

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 10, 2018
@roycaihw
Copy link
Member Author

Integration test added to check we expose the verbs

@roycaihw
Copy link
Member Author

/retest

nikhita
nikhita reviewed May 10, 2018
View reviewed changes
staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go
}

// make sure we don't get 405 Method Not Allowed from Patching CRD/status subresource
_, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().
Copy link
Member

@nikhita nikhita May 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

nit: if err = ...; err != nil

Multi-line casing, so it makes sense as-is. :)

nikhita
nikhita reviewed May 10, 2018
View reviewed changes
staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/etcd.go
@@ -160,12 +160,17 @@ type StatusREST struct {
store *genericregistry.Store
}

var _ = rest.Updater(&StatusREST{})
var _ = rest.Patcher(&StatusREST{})
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this line to the custom resource StatusREST as well? It currently satisfies the Patcher interface implicitly and supports get/update/patch but I think explicitly mentioning it is nicer and safer.

Also, StatusREST for deployments, statefulsets, etc does not implement Patcher explicitly as well...but I guess we should add it at least for the custom resource.

Copy link
Member Author

Choose a reason for hiding this comment

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

added

@nikhita
Copy link
Member

nikhita commented May 10, 2018

/area custom-resources

@lavalamp
Copy link
Member

cc @mbohlool

@liggitt
Copy link
Member

liggitt commented May 10, 2018

/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 10, 2018
@sttts
Copy link
Contributor

sttts commented May 11, 2018

@roycaihw please squash. Then lgtm and approved.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2018
@roycaihw
Copy link
Member Author

@sttts squashed

@sttts
Copy link
Contributor

sttts commented May 11, 2018

/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 May 11, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, roycaihw, sttts

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 May 11, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 769b7da into kubernetes:master May 11, 2018
@roycaihw roycaihw mentioned this pull request May 19, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request May 24, 2018
Merge pull request #64063 from roycaihw/support-get-status
74bcefc 
Automatic merge from submit-queue (batch tested with PRs 62756, 63862, 61419, 64015, 64063). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

apiservices/status and certificatesigningrequests/status support get+update+patch

**What this PR does / why we need it**:
Fix the remaining `/status` subresources that return 405 on GET and PATCH

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
ref #63619

**Release note**:

```release-note
apiservices/status and certificatesigningrequests/status now support GET and PATCH
```
k8s-publishing-bot added a commit to kubernetes/kube-aggregator that referenced this pull request May 24, 2018
@k8s-publishing-bot
Merge pull request #64063 from roycaihw/support-get-status
baacd71 
Automatic merge from submit-queue (batch tested with PRs 62756, 63862, 61419, 64015, 64063). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

apiservices/status and certificatesigningrequests/status support get+update+patch

**What this PR does / why we need it**:
Fix the remaining `/status` subresources that return 405 on GET and PATCH

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
ref kubernetes/kubernetes#63619

**Release note**:

```release-note
apiservices/status and certificatesigningrequests/status now support GET and PATCH
```

Kubernetes-commit: 74bcefc8b2bf88a2f5816336999b524cc48cf6c0
sttts pushed a commit to sttts/kube-aggregator that referenced this pull request Jun 8, 2018
@k8s-publishing-bot
Merge pull request #64063 from roycaihw/support-get-status
66a29a4 
Automatic merge from submit-queue (batch tested with PRs 62756, 63862, 61419, 64015, 64063). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

apiservices/status and certificatesigningrequests/status support get+update+patch

**What this PR does / why we need it**:
Fix the remaining `/status` subresources that return 405 on GET and PATCH

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
ref kubernetes/kubernetes#63619

**Release note**:

```release-note
apiservices/status and certificatesigningrequests/status now support GET and PATCH
```

Kubernetes-commit: 74bcefc8b2bf88a2f5816336999b524cc48cf6c0
k8s-publishing-bot added a commit to kubernetes/kube-aggregator that referenced this pull request Jun 8, 2018
@k8s-publishing-bot
Merge pull request #64063 from roycaihw/support-get-status
a852d3c 
Automatic merge from submit-queue (batch tested with PRs 62756, 63862, 61419, 64015, 64063). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

apiservices/status and certificatesigningrequests/status support get+update+patch

**What this PR does / why we need it**:
Fix the remaining `/status` subresources that return 405 on GET and PATCH

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
ref kubernetes/kubernetes#63619

**Release note**:

```release-note
apiservices/status and certificatesigningrequests/status now support GET and PATCH
```

Kubernetes-commit: 74bcefc8b2bf88a2f5816336999b524cc48cf6c0
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

@sttts sttts Awaiting requested review from sttts

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@sttts sttts

@liggitt liggitt

@nikhita nikhita

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/custom-resources 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@roycaihw @sttts @nikhita @lavalamp @liggitt @k8s-ci-robot @k8s-github-robot