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.

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

Eliminate nil/empty distinction for new v1 field #14532

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

ironcladlou
Copy link
Contributor

@ironcladlou ironcladlou commented Jun 8, 2017

Ensure there is no internal or external distinction between nil and
empty BrokerTemplateInstanceSpec.BindingIDs to enable maximum
compatibility with stored serialized types (e.g. protobufs).

Part of #13419.

@ironcladlou
Copy link
Contributor Author

[test]

@ironcladlou ironcladlou requested a review from liggitt June 12, 2017 14:41
@ironcladlou
Copy link
Contributor Author

/cc @liggitt

@liggitt
Copy link
Contributor

liggitt commented Jun 12, 2017

LGTM, would like second from @jim-minter as original author

@jim-minter
Copy link
Contributor

@ironcladlou thanks for doing this. Please additionally remove line https://github.com/openshift/origin/blob/master/test/extended/templates/templateservicebroker_e2e.go#L145 (BindingIDs: []string{}, and you'll be all set.

@jim-minter
Copy link
Contributor

also - is there any documentation / unit test that already exists or which can be put in place to prevent people like me making this mistake again?

@ironcladlou
Copy link
Contributor Author

@jim-minter

@ironcladlou thanks for doing this. Please additionally remove line https://github.com/openshift/origin/blob/master/test/extended/templates/templateservicebroker_e2e.go#L145 (BindingIDs: []string{}, and you'll be all set.

Thanks!

also - is there any documentation / unit test that already exists or which can be put in place to prevent people like me making this mistake again?

@liggitt had a test in a related upstream PR which ensured that slice fields missing omitempty serialized nil/empty to []; it seems possible to make a conceptually similar test in origin which ensures that all non-whitelisted API slice fields contain omitempty to enforce going forward.

@liggitt, does such a unit test seem valuable here?

Ensure there is no internal or external distinction between nil and
empty BrokerTemplateInstanceSpec.BindingIDs to enable maximum
compatibility with stored serialized typed (e.g. protobufs).
@ironcladlou
Copy link
Contributor Author

ironcladlou commented Jun 12, 2017

Hit a test flake. Went ahead and squashed to try one more run before merging.

ref #14573

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6bd4c9d

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2165/) (Base Commit: 6d3e523)

@ironcladlou
Copy link
Contributor Author

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6bd4c9d

@liggitt liggitt added this to the 3.6.0 milestone Jun 14, 2017
@smarterclayton
Copy link
Contributor

[severity:bug]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 15, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1001/) (Base Commit: 2df56bd) (Extended Tests: bug) (Image: devenv-rhel7_6361)

@openshift-bot openshift-bot merged commit db65cc1 into openshift:master Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants