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

Omit timeAdded from taint when empty #43016

Merged
merged 2 commits into from Sep 23, 2017
Merged

Omit timeAdded from taint when empty #43016

merged 2 commits into from Sep 23, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Mar 13, 2017 •
edited

Fixes omitempty portion of #42394

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

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 13, 2017
@liggitt liggitt mentioned this pull request Mar 13, 2017
Closed
@aveshagarwal
Copy link
Member

@kubernetes/sig-scheduling-pr-reviews

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 13, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 14, 2017 •
edited

cc @smarterclayton @davidopp to verify we can do this in 1.7 without backcompat issues

ignore the following, testing searching:

This PR may require API review.

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

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

@liggitt liggitt added this to the v1.7 milestone Mar 14, 2017
@liggitt liggitt added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Mar 14, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2017
@bsalamat
Copy link
Member

Looks good. Thanks, @liggitt.

@bsalamat
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2017
@mikedanese
Copy link
Member

/approve

k82cn
k82cn reviewed Mar 31, 2017
View reviewed changes
pkg/controller/daemon/daemoncontroller_test.go Outdated
@@ -58,17 +58,22 @@ var (
noScheduleTaints = []v1.Taint{{Key: "dedicated", Value: "user1", Effect: "NoSchedule"}}
)

func nowPointer() *metav1.Time {
Copy link
Member

Choose a reason for hiding this comment

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

Why not add this into metav1, named NowP() *Time? So other developer can use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not increase the exported API just yet

Copy link
Member

Choose a reason for hiding this comment

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

Just grep the code, there're some similar logic to get "now" pointer. I'll create a PR to clear them up by "NowP()" :).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it dilutes the metav1 package... there's no time.NowP() function in the stdlib, for example

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with that :)
Two lines to get "now" pointer seems boring :) ; just code style, no block comments to this PR :).

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 5, 2017
@bsalamat
Copy link
Member

bsalamat commented Apr 5, 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 Apr 5, 2017
@liggitt
Copy link
Member Author

liggitt commented Apr 5, 2017

@k8s-bot non-cri e2e test this
@k8s-bot verify test this

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2017
@k82cn
Copy link
Member

k82cn commented Aug 10, 2017

need rebase :).

@liggitt
Copy link
Member Author

liggitt commented Aug 10, 2017

@kubernetes/sig-scheduling-api-reviews @kubernetes/api-reviewers can I get approval on this before spending time keeping it rebased?

@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Aug 10, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 23, 2017
@k82cn
Copy link
Member

k82cn commented Sep 7, 2017

@liggitt , this was approved, can you help to rebase ? so we can merge it :).

@liggitt
Copy link
Member Author

liggitt commented Sep 7, 2017

rebased, will leave up to @kubernetes/sig-scheduling-api-reviews whether this is important for 1.8

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2017
@timothysc
Copy link
Member

/cc @gmarek

gmarek
gmarek reviewed Sep 12, 2017
View reviewed changes
staging/src/k8s.io/api/core/v1/types.go
@@ -2466,7 +2466,7 @@ type Taint struct {
// TimeAdded represents the time at which the taint was added.
// It is only written for NoExecute taints.
// +optional
TimeAdded metav1.Time `json:"timeAdded,omitempty" protobuf:"bytes,4,opt,name=timeAdded"`
TimeAdded *metav1.Time `json:"timeAdded,omitempty" protobuf:"bytes,4,opt,name=timeAdded"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a breaking API change in beta type?

Copy link
Member Author

Choose a reason for hiding this comment

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

wire serialization remains compatible.

the old version encoded zero values to:

  • json: timeAdded: null
  • proto: 0x22 tag with zero length value

the new version omits zero values from the wire format

the new version successfully decodes zero values from the old version

the old version treats missing values as the zero value when decoding

@bsalamat
Copy link
Member

As long as we are backward compatible, I am fine with the change.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2017
@k82cn
Copy link
Member

k82cn commented Sep 13, 2017

I'm fine to merge this in 1.9 :).

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, k82cn, liggitt, mikedanese

Associated issue: 42394

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

@liggitt liggitt added this to the v1.9 milestone Sep 13, 2017
@fejta-bot
Copy link

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

Review the full test history for this PR.

1 similar comment
@fejta-bot
Copy link

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

Review the full test history for this PR.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 43016, 50503, 51281, 51518, 51582). If you want to cherry-pick this change to another branch, please follow the instructions here..

@k8s-github-robot k8s-github-robot merged commit 1c0f22e into kubernetes:master Sep 23, 2017
@k8s-ci-robot
Copy link
Contributor

@liggitt: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-unit fe6af20 link /test pull-kubernetes-unit
pull-kubernetes-e2e-gce-bazel fe6af20 link /test pull-kubernetes-e2e-gce-bazel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@liggitt liggitt deleted the time-added-pointer branch September 25, 2017 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmarek gmarek gmarek left review comments

@k82cn k82cn k82cn approved these changes

Assignees

@jsafrane jsafrane

@davidopp davidopp

@bsalamat bsalamat

@k82cn k82cn

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 lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.

None yet