Skip to content

Add json/protobuf/yaml fixtures#78309

Merged
k8s-ci-robot merged 10 commits intokubernetes:masterfrom
liggitt:proto-fixtures
Jun 2, 2019
Merged

Add json/protobuf/yaml fixtures#78309
k8s-ci-robot merged 10 commits intokubernetes:masterfrom
liggitt:proto-fixtures

Conversation

@liggitt
Copy link
Copy Markdown
Member

@liggitt liggitt commented May 24, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Adds roundtrip testing to/from persisted fixtures we can snapshot per release.

This will give us confidence bumps of proto libraries aren't breaking backwards compatibility with old serialized data.

Special notes for your reviewer:

Follow-ups:

Does this PR introduce a user-facing change?:

NONE

/sig api-machinery
/sig architecture
/cc @smarterclayton

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 24, 2019
@liggitt liggitt force-pushed the proto-fixtures branch 2 times, most recently from 8551412 to d515c9f Compare May 24, 2019 17:10
@liggitt liggitt added this to the v1.15 milestone May 28, 2019
Copy link
Copy Markdown
Contributor

@smarterclayton smarterclayton left a comment

Choose a reason for hiding this comment

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

Some structural things

@liggitt liggitt changed the title WIP - Add json/protobuf fixtures Add json/protobuf fixtures May 29, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2019
@liggitt liggitt changed the title Add json/protobuf fixtures Add json/protobuf/yaml fixtures May 29, 2019
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented May 29, 2019

addressed comments required for initial merge. I want to get this in asap to get primary API types protected, and do the additional API dirs as follow-ups

@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented May 29, 2019

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 29, 2019
@fejta-bot
Copy link
Copy Markdown

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Jun 2, 2019

/hold cancel
/test pull-kubernetes-bazel-rest

merge queue is emptyish. will need to update this for a few API PRs that merged

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2019
@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Jun 2, 2019

/test pull-kubernetes-bazel-test

@fejta-bot
Copy link
Copy Markdown

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

liggitt added 10 commits June 2, 2019 00:13
        --- FAIL: TestCompatibility/core.v1.ListOptions/v1.14.0 (0.02s)
            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 10 identical lines
                  	"  7: 6780787122834727873",
                  	`  8: "5"`,
                + 	"  9: 0",
                  	"}",
                  	`3: ""`,
                  	... // 2 identical lines
                  }, "\n")

Caused by https://github.com/kubernetes/kubernetes/pull/74074/files#diff-eca3b8d856fa2e661f6da91b61de5e76R364
        --- FAIL: TestCompatibility/admission.k8s.io.v1beta1.AdmissionReview/v1.14.0 (0.02s)
            compatibility.go:460: json differs
            compatibility.go:461:   strings.Join({
                  	... // 31 identical lines
                  	`    "object": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                  	`    "oldObject": {"apiVersion":"example.com/v1","kind":"CustomType","spec":{"replicas":1},"status":{"available":1}},`,
                - 	`    "dryRun": true`,
                + 	`    "dryRun": true,`,
                + 	`    "options": null`,
                  	"  },",
                  	`  "response": {`,
                  	... // 33 identical lines
                  }, "\n")

            compatibility.go:466: yaml differs
            compatibility.go:467:   strings.Join({
                  	... // 23 identical lines
                  	"      available: 1",
                  	"  operation: 祈¡ıŵDz廔ȇ{sŊƏp饏姥呄鐊",
                + 	"  options: null",
                  	"  resource:",
                  	`    group: "5"`,
                  	... // 38 identical lines
                  }, "\n")

            compatibility.go:472: proto differs
            compatibility.go:474:   strings.Join({
                  	... // 37 identical lines
                  	"    }",
                  	"    11: 1",
                + 	`    12: ""`,
                + 	`    15: ""`,
                  	"  }",
                  	"  2 {",
                  	... // 36 identical lines
                  }, "\n")

Null `options` in json and additional proto tag 12 caused by kubernetes#77563 (comment)

Proto tag 15 caused by https://github.com/kubernetes/kubernetes/pull/78135/files#diff-3bc4acaf71b6b50648da046287017c54R82
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Jun 2, 2019

rebased/regenerated fixtures for master

@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Jun 2, 2019

retagging after rebase

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2019
@fejta-bot
Copy link
Copy Markdown

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link
Copy Markdown

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@liggitt
Copy link
Copy Markdown
Member Author

liggitt commented Jun 2, 2019

/retest

@fejta-bot
Copy link
Copy Markdown

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants