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

Make serverAddressByClientCIDRs in discovery API optional #61963

Merged
merged 2 commits into from
May 25, 2018
Merged

Make serverAddressByClientCIDRs in discovery API optional #61963

merged 2 commits into from
May 25, 2018

Conversation

roycaihw
Copy link
Member

What this PR does / why we need it:
See #61868

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #61868

Special notes for your reviewer:
WIP: I'm having trouble updating swagger-spec using our update scripts. Thinking about removing swagger-spec from our code base as it has long passed deprecation. Sending this PR now to see the test results.

Release note:

Property `serverAddressByClientCIDRs` in `metav1.APIGroup` (discovery API) now become optional instead of required

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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. labels Mar 30, 2018
@k8s-ci-robot k8s-ci-robot requested review from eparis and therc March 30, 2018 22:50
@roycaihw
Copy link
Member Author

roycaihw commented Apr 2, 2018

/retest

@lavalamp
Copy link
Member

lavalamp commented Apr 2, 2018

/assign @mbohlool

@roycaihw
Copy link
Member Author

/retest

@mbohlool
Copy link
Contributor

/lgtm /approve

@mbohlool
Copy link
Contributor

/test pull-kubernetes-e2e-gce

@mbohlool
Copy link
Contributor

We need more of these PRs. There are more properties (mostly lists) that we return null in kubernetes for them and assumption is null is empty. I think you can compile a list of them if you look through python client issues but it would be nice to do a sweep on all arrays/maps in return values in our types and check if they can be null and add optional for them.

Thanks for taking care of this @roycaihw

@roycaihw roycaihw changed the title [WIP] Make serverAddressByClientCIDRs in discovery API optional Make serverAddressByClientCIDRs in discovery API optional Apr 16, 2018
@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 Apr 16, 2018
@roycaihw
Copy link
Member Author

would like to merge this after swagger-spec gets removed, to avoid confusion

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2018
@roycaihw roycaihw mentioned this pull request May 21, 2018
Open
@roycaihw
Copy link
Member Author

Since I saw update-openapi-spec and update-swagger-spec diverging already in some other PRs, it might worth getting this PR in ignoring swagger-spec

/hold cancel
/assign @caesarxuchao
for approval

@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 May 21, 2018
@mbohlool
Copy link
Contributor

@roycaihw I agree. We no longer keep an updated swagger 1.2 spec. It should be removed by our deprecation policy does not allow removing it yet.

@mbohlool
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2018
caesarxuchao
caesarxuchao reviewed May 22, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go Outdated
@@ -799,6 +799,7 @@ type APIGroup struct {
// The server returns only those CIDRs that it thinks that the client can match.
// For example: the master will return an internal IP CIDR only, if the client reaches the server using an internal IP.
// Server looks at X-Forwarded-For header or X-Real-Ip header or request.RemoteAddr (in that order) to get the client IP.
// +optional
ServerAddressByClientCIDRs []ServerAddressByClientCIDR `json:"serverAddressByClientCIDRs" protobuf:"bytes,4,rep,name=serverAddressByClientCIDRs"`
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 the json tag needs the omitempty.

@roycaihw roycaihw force-pushed the optional-saddress-ccidr branch from b913a97 to a14f423 Compare May 22, 2018 01:08
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2018
@roycaihw
Copy link
Member Author

I think the json tag needs the omitempty.

@caesarxuchao That's a great point. Updated.

Also it seems that update-swagger-spec is triggered by omitempty.

@caesarxuchao
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 May 23, 2018
@lavalamp
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, lavalamp, mbohlool, roycaihw

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 24, 2018
@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.

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

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.

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

@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 61963, 64279, 64130, 64125, 64049). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f091073 into kubernetes:master May 25, 2018
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

@eparis eparis Awaiting requested review from eparis

@therc therc Awaiting requested review from therc

Assignees

@mbohlool mbohlool

@caesarxuchao caesarxuchao

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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discovery API responding with null for required property
7 participants
@roycaihw @lavalamp @mbohlool @caesarxuchao @k8s-ci-robot @fejta-bot @k8s-github-robot