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

Move to upstream Sddl/SecurityAttribute functions #165

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zx2c4
Copy link

@zx2c4 zx2c4 commented Sep 30, 2019

These have been moved into x/sys/windows entirely.

These have been moved into x/sys/windows entirely.

Signed-off-by: Jason A. Donenfeld <[email protected]>
@msftclas
Copy link

msftclas commented Sep 30, 2019

CLA assistant check
All CLA requirements met.

@jterry75
Copy link
Contributor

@zx2c4 - The change looks good but I am worried about just deleting the exported functions. I think that we should likely go through a deprecation period. @kevpar / @jstarks Thoughts?

@jstarks
Copy link
Member

jstarks commented Sep 30, 2019

We should leave the existing functions but have them call through to the windows package (and only if the semantics are exactly the same).

@thaJeztah
Copy link
Contributor

Heads up: from the discussion on moby/moby#40021 (comment)

LGTM, but I'd wait for the outcome of the discussion on golang/go#34610 where the decision might be to revert the change to SecurityAttributes and introduce a new struct type with a different name.

So it might be good to hold off on this one

@katiewasnothere
Copy link
Contributor

Is there any update on this?

@thaJeztah
Copy link
Contributor

My comment above is now outdated; it was decided not to revert (moby/moby#40021 (comment))

This is good to go now that golang/go#34610 has been closed and it was decided there will be no revert (golang/go#34610 (comment))

So from that perspective this should be good to go (I'm not a maintainer here though!)

@katiewasnothere
Copy link
Contributor

@jstarks do we want these changes?

We need to update (for example) hcsshim to no longer use out of date functions such as SddlToSecurityDescriptor so we can update the x/sys dependency there. Might be good then to update this here for consistency.

@kolyshkin
Copy link

We need to update (for example) hcsshim to no longer use out of date functions such as SddlToSecurityDescriptor so we can update the x/sys dependency there. Might be good then to update this here for consistency.

It was a simple fix, unless I'm missing something; please see the second commit in microsoft/hcsshim#828

@katiewasnothere
Copy link
Contributor

I've made a new PR #172 to address a few other things.

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.

7 participants