-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
pkg/ansible/controller,pkg/helm/controller; Enable status subresource by default #787
Conversation
Hi @dymurray. Thanks for your PR. I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. |
@dymurray Before we merge this we need to make sure that we add some migration steps to some sort of doc to describe what it looks like if you're using a CRD without sub-resources and what to do to fix it. |
Just correcting Shawn's comment, this requires v1.11 of Kubernetes by default: |
fixes: #809 as we're turning on the status subresource by default which is beta in k8s 1.11 per kubernetes/kubernetes#63598 |
d8f5560
to
cdc6f97
Compare
@hasbro17 I can confirm that bringing up a v1.11.0 Kubernetes Cluster the status subresource is on by default. |
@dymurray Have you looked into the Helm error in this build at all? I'm seeing the same error trying to add a status subresource to a CRD. |
@joelanford I haven't dug into it just yet but your link looks promising. I will report back if I find anything. It only began happening when I added the status writer to the helm controller. |
/hold cancel |
@dymurray Can you please update the PR title and description to more clearly say that we're enabling the status subresource by default. |
791ca57
to
ba3a7e6
Compare
Helm CI tests are related to the bug Joe mentioned above. I ran the tests with Edit: I bumped the client libraries to 1.11.5 to resolve this and updated Gopkg.* files |
068e79d
to
73a20c7
Compare
1c7debf
to
1990993
Compare
I have removed all client library bumps from this PR because of #807 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @hasbro17, added the note to the changelog. |
7e9affe
to
2ab3ec6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after nit
My bad, I forgot to squash before merging. |
Description of the change:
Enable use of status subresource by default in operator-sdk.
Ansible:
Adds support for hybrid managed status between AO and user's Ansible. Get's latest version of resource to ensure that we are updating the latest version of the resource.
Motivation for the change:
In Kubernetes v1.11.0 the status subresource is enabled by default and so this PR enables the status subresource by default in operator-sdk.
Ansible:
Hybrid use case panics when user updates status from Ansible task.
Closes #809