Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Only validate status on updates #1972

Merged
merged 1 commit into from
Apr 30, 2018

Conversation

jpeeler
Copy link

@jpeeler jpeeler commented Apr 23, 2018

This basically ensures that spec validations don't include status.

Closes #1960

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 23, 2018
@jberkhahn
Copy link
Contributor

This looks good. Interesting that we weren't already testing validateServiceBindingStatus() anywhere in the test.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 25, 2018
@jpeeler
Copy link
Author

jpeeler commented Apr 25, 2018

@n3wscott I think what I was missing was the fuzzer needed adjusting. You're probably a good person to look over this too once all this is green.

This basically ensures that spec validations don't include status.
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 26, 2018
@jpeeler
Copy link
Author

jpeeler commented Apr 26, 2018

Was wrong about the fuzzer.

@jpeeler jpeeler changed the title WIP: Only validate status on updates Only validate status on updates Apr 26, 2018
@MHBauer
Copy link
Contributor

MHBauer commented Apr 27, 2018

@jpeeler there's no fuzzer changes here. Are comments on the right PR?

@pmorie
Copy link
Contributor

pmorie commented Apr 27, 2018

Yes, Jeff recut the PR not to introduce a new default.

@pmorie
Copy link
Contributor

pmorie commented Apr 27, 2018

Jeff is out today, but we have been discussing this PR offline. He tested this by:

  • Running API server without controller
  • Setting up test scenario by using etcdctl to replace the record for a Binding with a version without UnbindStatus set
  • calling kubectl replace on the instance with itself to simulate storage migration

@pmorie pmorie added the LGTM1 label Apr 27, 2018
@jpeeler
Copy link
Author

jpeeler commented Apr 30, 2018

Everything Paul said is correct, this PR is ready to go.

@jboyd01 jboyd01 added the LGTM2 label Apr 30, 2018
@jboyd01 jboyd01 merged commit da71f44 into kubernetes-retired:master Apr 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants