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

fix the inconsistent description of TopologyKey in PodAffinityTerm #54797

Merged
merged 2 commits into from
Nov 11, 2017
Merged

fix the inconsistent description of TopologyKey in PodAffinityTerm #54797

merged 2 commits into from
Nov 11, 2017

Conversation

guangxuli
Copy link
Contributor

What this PR does / why we need it:
Clarify the confusing of inconsistent description.
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Just fix #54276
Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 30, 2017
@xiangpengzhao
Copy link
Contributor

/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 Oct 30, 2017
@guangxuli
Copy link
Contributor Author

/retest

@guangxuli guangxuli force-pushed the fix_inconsistent_description branch 2 times, most recently from 2b8fc20 to 350bec6 Compare October 31, 2017 10:45
@guangxuli
Copy link
Contributor Author

/retest

@caesarxuchao
Copy link
Member

/assign @aveshagarwal

@guangxuli
Copy link
Contributor Author

/test pull-kubernetes-verify

@guangxuli
Copy link
Contributor Author

/retest

@guangxuli
Copy link
Contributor Author

/test pull-kubernetes-unit

@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 Nov 1, 2017
@guangxuli
Copy link
Contributor Author

all tests are green.
@aveshagarwal @caesarxuchao @saad-ali PTAL.

aveshagarwal
aveshagarwal reviewed Nov 1, 2017
View reviewed changes
staging/src/k8s.io/api/core/v1/types.go Outdated
// For PreferredDuringScheduling pod anti-affinity, empty topologyKey is interpreted as "all topologies"
// ("all topologies" here means all the topologyKeys indicated by scheduler command-line argument --failure-domains);
// for affinity and for RequiredDuringScheduling pod anti-affinity, empty topologyKey is not allowed.
// Empty topologyKey is not allowed.
// +optional
TopologyKey string `json:"topologyKey,omitempty" protobuf:"bytes,3,opt,name=topologyKey"`
Copy link
Member

Choose a reason for hiding this comment

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

i think you need to remove omitempty too unless there is a default value assigned. could you check it?

Copy link
Contributor Author

@guangxuli guangxuli Nov 2, 2017 •
edited

Choose a reason for hiding this comment

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

Agree, i didn't realize that point,thanks.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 2, 2017
@guangxuli
Copy link
Contributor Author

@aveshagarwal ptal.

@guangxuli
Copy link
Contributor Author

kindly ping @caesarxuchao @aveshagarwal

@aveshagarwal
Copy link
Member

lgtm.

@guangxuli
Copy link
Contributor Author

guangxuli commented Nov 3, 2017 •
edited

@smarterclayton @liggitt
need your approve,thanks.

caesarxuchao
caesarxuchao reviewed Nov 3, 2017
View reviewed changes
staging/src/k8s.io/api/core/v1/types.go Outdated
// For PreferredDuringScheduling pod anti-affinity, empty topologyKey is interpreted as "all topologies"
// ("all topologies" here means all the topologyKeys indicated by scheduler command-line argument --failure-domains);
// for affinity and for RequiredDuringScheduling pod anti-affinity, empty topologyKey is not allowed.
// Empty topologyKey is not allowed.
// +optional
Copy link
Member

Choose a reason for hiding this comment

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

Remove the +optional

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise lgtm.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2017
@guangxuli
Copy link
Contributor Author

hi @caesarxuchao do you fully sure this field is Mandatory? A little confusing that in our internal api type definition, topologyKey is still optional.

@liggitt
Copy link
Member

liggitt commented Nov 7, 2017

In all validation cases I could find, an API object that leaves it empty fails validation. I'm tentatively ok with this, assuming due diligence to sweep all places this field is used/validated to ensure this isn't tightening validation on any persisted objects

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

thanks @liggitt hmm, do you mean in this PR we could remove the optional? I have no plan to sweep all the places, just only confirm the api definition rules, i am not fully familiar with this area. :)

@guangxuli
Copy link
Contributor Author

/retest

@guangxuli
Copy link
Contributor Author

@caesarxuchao @aveshagarwal @liggitt PTAL. thanks.

@caesarxuchao
Copy link
Member

@liggitt the tightening of the validation was done in #49976, this one is just updating the api description to reflect the truth. Do you have other concerns?

@liggitt
Copy link
Member

liggitt commented Nov 10, 2017

Sgtm

@guangxuli
Copy link
Contributor Author

guangxuli commented Nov 10, 2017 •
edited

Any comments for this PR? @liggitt @aveshagarwal @caesarxuchao @xiangpengzhao

@aveshagarwal
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 Nov 10, 2017
@liggitt
Copy link
Member

liggitt commented Nov 10, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, guangxuli, liggitt

Associated issue: 54276

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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2017
@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. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 6843654 into kubernetes:master Nov 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caesarxuchao caesarxuchao caesarxuchao left review comments

@aveshagarwal aveshagarwal aveshagarwal left review comments

Assignees

@caesarxuchao caesarxuchao

@aveshagarwal aveshagarwal

@saad-ali saad-ali

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-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

podAffinityTerm.topologyKey is required
8 participants
@guangxuli @xiangpengzhao @caesarxuchao @aveshagarwal @liggitt @k8s-github-robot @saad-ali @k8s-ci-robot