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

export unstructured helper function nestedFieldNoCopy #62287

Merged
merged 1 commit into from
Apr 10, 2018
Merged

export unstructured helper function nestedFieldNoCopy #62287

merged 1 commit into from
Apr 10, 2018

Conversation

skriss
Copy link
Contributor

@skriss skriss commented Apr 9, 2018 •
edited
Loading

Signed-off-by: Steve Kriss steve@heptio.com

What this PR does / why we need it: Export the unstructured helper function nestedFieldNoCopy. This enables checking for existence of nested fields without requiring a deep-copy via JSON.

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

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 9, 2018
@k8s-ci-robot k8s-ci-robot requested review from janetkuo and therc April 9, 2018 18:00
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 9, 2018
@ncdc
Copy link
Member

ncdc commented Apr 9, 2018

/ok-to-test
@kubernetes/sig-api-machinery-pr-reviews

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 9, 2018
@ncdc ncdc self-assigned this Apr 9, 2018
@ncdc
Copy link
Member

ncdc commented Apr 9, 2018

/assign @deads2k
/lgtm

@k8s-ci-robot k8s-ci-robot added 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. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 9, 2018
@skriss
Copy link
Contributor Author

skriss commented Apr 9, 2018

/retest

deads2k
deads2k reviewed Apr 9, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/helpers.go Outdated
// it does not exist, or (false, error) if an error is encountered traversing
// obj.
func NestedSliceExists(obj map[string]interface{}, fields ...string) (bool, error) {
val, found, err := nestedFieldNoCopy(obj, fields...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just expose this function and let people do what they need to do. These helpers seem overly restrictive for how you can use a thing.

@sttts
Copy link
Contributor

sttts commented Apr 10, 2018

/assign @ash2k

@sttts
Copy link
Contributor

sttts commented Apr 10, 2018

Lgtm. But would like to hear @ash2k's opinion. Holding until then.

/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 10, 2018
@ash2k
Copy link
Member

ash2k commented Apr 10, 2018

I think we can merge this as is but I agree with @deads2k - we could expose the underlying helper functions. Maybe do both things - add these two helpers and make those private functions public?

@deads2k
Copy link
Contributor

deads2k commented Apr 10, 2018

I think we can merge this as is but I agree with @deads2k - we could expose the underlying helper functions. Maybe do both things - add these two helpers and make those private functions public?

Unused helpers always seem odd. Having been responsible for a few exposed functions that could be private, I'm fine with exposing nestedFieldNoCopy to allow this use-case to be built externally, but to my knowledge we don't have any *Exists functions at all. We can expose a useful function externally, limit our surface area to what we need, and the external integration can still be built.

I'm assuming it's a external thing or there would be a change to use these functions somewhere in the code-base, right?

@sttts
Copy link
Contributor

sttts commented Apr 10, 2018

we don't have any *Exists functions at all.

Because the non-object getters give you the exists bool as well. For slices and maps we distinguish between "copy" and "no-copy".

@skriss
Copy link
Contributor Author

skriss commented Apr 10, 2018

yes, this is for an external integration. I'd be okay with exposing nestedFieldNoCopy if that's what everyone prefers since that addresses the main limitation we're trying to address (can't tell if a nested field exists without a deep copy).

@deads2k
Copy link
Contributor

deads2k commented Apr 10, 2018

yes, this is for an external integration. I'd be okay with exposing nestedFieldNoCopy if that's what everyone prefers since that addresses the main limitation we're trying to address (can't tell if a nested field exists without a deep copy).

Let's do that.

@skriss skriss force-pushed the add-unstructured-exists-helpers branch from 7125f0f to d2e2800 Compare April 10, 2018 17:42
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2018
@skriss skriss changed the title add NestedSliceExists and NestedMapExists to unstructured helpers export unstructured helper nestedFieldNoCopy Apr 10, 2018
@skriss skriss changed the title export unstructured helper nestedFieldNoCopy export unstructured helper function nestedFieldNoCopy Apr 10, 2018
@skriss
Copy link
Contributor Author

skriss commented Apr 10, 2018

/retest

@ncdc
Copy link
Member

ncdc commented Apr 10, 2018

@sttts @ash2k @deads2k better? Ok to remove the hold?

@deads2k
Copy link
Contributor

deads2k commented Apr 10, 2018

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ncdc, skriss

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 Apr 10, 2018
@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 c7349d8 into kubernetes:master Apr 10, 2018
@wenjiaswe
Copy link
Contributor

cc @roycaihw

@skriss skriss deleted the add-unstructured-exists-helpers branch April 18, 2018 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@janetkuo janetkuo Awaiting requested review from janetkuo

@therc therc Awaiting requested review from therc

Assignees

@ncdc ncdc

@ash2k ash2k

@deads2k deads2k

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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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.
8 participants
@skriss @ncdc @sttts @ash2k @deads2k @k8s-ci-robot @k8s-github-robot @wenjiaswe