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

Update photon controller go SDK in vendor code. #43108

Merged
merged 1 commit into from
Mar 17, 2017
Merged

Update photon controller go SDK in vendor code. #43108

merged 1 commit into from
Mar 17, 2017

Conversation

luomiao
Copy link

@luomiao luomiao commented Mar 14, 2017 •
edited
Loading

What this PR does / why we need it:
Update photon controller go SDK in vendor code.

Which issue this PR fixes:
fixes ##43042

Special notes for your reviewer:
Can we mark this PR as bug fix and with 1.6 milestone?
Since photon controller is using a new version of this go SDK. Without this change, the current k8s will break with latest released photon controller. Thanks.

Release note:
Compatible with Photon Controller v1.1.1 release.

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

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Mar 14, 2017
@luomiao luomiao force-pushed the update-photon-go-sdk-vendor branch from 2f11b76 to 1439ea2 Compare March 15, 2017 00:21
@lavalamp lavalamp added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 15, 2017
@lavalamp
Copy link
Member

Please edit the release note section to something stating the photon versions it's compatible with.

Do you expect to force every k8s cluster running on photon to upgrade? I'm not against merging this for 1.6 but that's probably not the bulk of the problem. It seems like a bug in photon if they've broken older clients?

Since this is changing a library that isn't used in any of the continuous testing (that I'm aware of) I think it's fine to merge, it shouldn't break anything--however it's on you @luomiao to have tested that the photon integration still works after this :)

@ethernetdan is apparently making calls about in 1.6 or not. @ethernetdan, can you add the 1.6 milestone if you don't see anything wrong with the above reasoning? Thanks.

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, luomiao

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 Mar 15, 2017
@luomiao
Copy link
Author

luomiao commented Mar 15, 2017

@lavalamp Hi Daniel, thanks for evaluating the situation.
Photon Controller's support of k8s cluster with persistent volume features is not officially released yet and that's why there are many updates in the photon go SDK which breaks the current cloud provider. Luckily there are no old client in production mode that would be affected by this update. We are also working on integration tests to make things more stable towards future release :)

@ethernetdan ethernetdan added this to the v1.6 milestone Mar 16, 2017
@luomiao
Copy link
Author

luomiao commented Mar 17, 2017

@k8s-bot bazel test this
@k8s-bot unit test this

liggitt
liggitt reviewed Mar 17, 2017
View reviewed changes
vendor/github.com/vmware/photon-controller-go-sdk/SSPI/sspi.go

var (
secur32_dll = syscall.NewLazyDLL("secur32.dll")
initSecurityInterface = secur32_dll.NewProc("InitSecurityInterfaceW")
Copy link
Member

@liggitt liggitt Mar 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

is this dll and proc guaranteed to exist on all supported windows platforms? (since you're calling it in the init() function, a panic would prevent the binary from starting, even if photon was never used

Copy link
Author

Choose a reason for hiding this comment

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

Hi Jordan. Thanks for your review. I have talked to Photon team who's maintaining this go sdk. We think this usage should be safe for 1) this will only be built on Windows and 2) this refers to standard Windows stuff. Please let us know if you still have concern about this part of code.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for following up. If it's a standard lib on all versions of windows we support, that's seems ok, I just wanted to check.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

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

@liggitt liggitt liggitt left review comments

Assignees

@lavalamp lavalamp

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.
7 participants
@luomiao @k8s-reviewable @lavalamp @k8s-github-robot @liggitt @ethernetdan @k8s-ci-robot