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

change apps/v1beta2 StatefulSet observedGeneration from a pointer to an int for consistency #49607

Merged
merged 2 commits into from
Aug 3, 2017
Merged

change apps/v1beta2 StatefulSet observedGeneration from a pointer to an int for consistency #49607

merged 2 commits into from
Aug 3, 2017

Conversation

dixudx
Copy link
Member

@dixudx dixudx commented Jul 26, 2017 •
edited
Loading

What this PR does / why we need it:
change the StatefulSet observedGeneration from a pointer to an int for consistency

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #49623
xref #49135

Special notes for your reviewer:
/cc @janetkuo @foxish @kow3ns

Release note:

change apps/v1beta2 StatefulSet observedGeneration (optional field) from a pointer to an int for consistency

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 26, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @dixudx. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 26, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2017
@dixudx dixudx force-pushed the change_StS_observedGeneration_to_int branch from ac88296 to 9e131e2 Compare July 26, 2017 08:23
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2017
wojtek-t
wojtek-t reviewed Jul 27, 2017
View reviewed changes
staging/src/k8s.io/api/apps/v1beta2/types.go
@@ -225,7 +225,7 @@ type StatefulSetStatus struct {
// observedGeneration is the most recent generation observed for this StatefulSet. It corresponds to the
// StatefulSet's generation, which is updated on mutation by the API Server.
// +optional
ObservedGeneration *int64 `json:"observedGeneration,omitempty" protobuf:"varint,1,opt,name=observedGeneration"`
ObservedGeneration int64 `json:"observedGeneration,omitempty" protobuf:"varint,1,opt,name=observedGeneration"`
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this? Isn't it a backward incompatible change?
@deads2k @smarterclayton

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do this? Isn't it a backward incompatible change?
@deads2k @smarterclayton

Isn't the v1beta2 serialization net new in 1.8? If it is, then the proto serialization won't be a problem. Not sure how you'd roundtrip through it though. The compatibility of that bit depends on the meaning of nil versus 0 in the API itself.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, apps/v1beta2 is new and WIP so it's safe to break it.

@kow3ns kow3ns mentioned this pull request Jul 27, 2017
Closed
11 tasks
@mbohlool
Copy link
Contributor

/unassign mbohlool

Looks good from API point of view (backward compatibility is not an issue here).

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2017
@dixudx dixudx force-pushed the change_StS_observedGeneration_to_int branch from 9e131e2 to c11737e Compare July 29, 2017 04:57
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2017
@kow3ns
Copy link
Member

kow3ns commented Aug 1, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 1, 2017
@kow3ns
Copy link
Member

kow3ns commented Aug 1, 2017

@dixudx You will probably need to update the fuzzer to support round trip conversions from a pointer to a integer value and vice versa.

@dixudx
Copy link
Member Author

dixudx commented Aug 2, 2017

/retest

@dixudx dixudx force-pushed the change_StS_observedGeneration_to_int branch from c11737e to 9277997 Compare August 2, 2017 05:01
@dixudx
Copy link
Member Author

dixudx commented Aug 2, 2017

@janetkuo @foxish @wojtek-t Need your approval. Thanks.

@wojtek-t
Copy link
Member

wojtek-t commented Aug 2, 2017

/approve

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

kow3ns commented Aug 2, 2017

@dixudx One thing, can you update the release note to indicate that the observedGeneration field of the StatefulSetStatus Kind in the apps/v1beta2 group is an optional integer and not a pointer.

@dixudx
Copy link
Member Author

dixudx commented Aug 2, 2017

@wojtek-t Need your lgtm also. Thanks.

@dixudx dixudx changed the title change the StatefulSet observedGeneration from a pointer to an int for consistency change apps/v1beta2 StatefulSet observedGeneration from a pointer to an int for consistency Aug 2, 2017
@dixudx
Copy link
Member Author

dixudx commented Aug 2, 2017

@kow3ns Done.

@kow3ns
Copy link
Member

kow3ns commented Aug 2, 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 Aug 2, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, kow3ns, wojtek-t

Associated issue: 49623

The full list of commands accepted by this bot can be found here.

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
Copy link

Automatic merge from submit-queue (batch tested with PRs 50000, 49954, 49943, 50018, 49607)

@k8s-github-robot k8s-github-robot merged commit 6f99ed4 into kubernetes:master Aug 3, 2017
@dixudx dixudx deleted the change_StS_observedGeneration_to_int branch August 3, 2017 03:10
@dixudx dixudx mentioned this pull request Jan 11, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojtek-t wojtek-t wojtek-t left review comments

@deads2k deads2k deads2k left review comments

@kow3ns kow3ns kow3ns approved these changes

@foxish foxish Awaiting requested review from foxish

@janetkuo janetkuo Awaiting requested review from janetkuo

Assignees

@janetkuo janetkuo

@wojtek-t wojtek-t

@kow3ns kow3ns

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/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.

change apps/v1beta2 StatefulSet observedGeneration from a pointer to an int for consistency
8 participants
@dixudx @k8s-ci-robot @mbohlool @kow3ns @wojtek-t @k8s-github-robot @janetkuo @deads2k