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.

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

RemainingItemCount is serialized in proto messages even when unset #78318

Closed
liggitt opened this issue May 24, 2019 · 2 comments · Fixed by #78327
Closed

RemainingItemCount is serialized in proto messages even when unset #78318

liggitt opened this issue May 24, 2019 · 2 comments · Fixed by #78327
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Milestone

Comments

@liggitt
Copy link
Member

liggitt commented May 24, 2019

What happened:
As part of adding skew testing for serialized API responses, we discovered a diff in serialized proto responses when roundtripping data from 1.14:

    --- FAIL: TestRoundTripExternalTypes/.v1.Status (0.02s)
        --- FAIL: TestRoundTripExternalTypes/.v1.Status/fixtures-v1.14.0 (0.02s)
            fixtures.go:195: proto data differed from fixture
            fixtures.go:205: fixture vs reserialized:
                  strings.Join({
                  	... // 7 identical lines
                  	`    2: "8551505577692297499"`,
                  	`    3: ""`,
                + 	"    4: 0",
                  	"  }",
                  	"  2 {",
                  	... // 21 identical lines
                  }, "\n")

This is due to the RemainingItemCount field in ListMeta, which is always serialized in proto messages, because it is not a pointer.

What you expected to happen:
Proto messages do not include the optional field when unset.

How to reproduce it (as minimally and precisely as possible):

  • Check out Add json/protobuf/yaml fixtures #78309
  • Run go test ./vendor/k8s.io/api -run TestRoundTripExternalTypes//1.14 (tests round-tripping serialized API content for all types we shipped in 1.14)

This needs to be resolved before 1.15 ships.

/milestone v1.15
/kind bug
/sig api-machinery
/priority important-soon
/assign @caesarxuchao

@liggitt liggitt added the kind/bug Categorizes issue or PR as related to a bug. label May 24, 2019
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 24, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 24, 2019
@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 24, 2019
@caesarxuchao
Copy link
Member

What buggy behavior will an end user experience?

@liggitt
Copy link
Member Author

liggitt commented May 25, 2019

What buggy behavior will an end user experience?

the user would be unable to tell whether the server had returned a value for the field or not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants