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

[v9] Backport #12530, #12531, #12405, #12615, and #12684 #14012

Merged

Conversation

ibeckermayer
Copy link
Contributor

Backport #12530, #12531, #12405, #12615, and #12684 to branch/v9

This catches branch/v9 up with all of the backports necessary to make it on par with branch/v10 in terms of directory sharing changes.

Initially I wasn't planning to backport directory sharing stuff to branch/v9, however I've later come to realize that directory sharing development is making certain fundamental changes to how the rdp client works in master/branch/v10, which is making backporting to branch/v9 a lot more fragile and in need of manual assistance (e.g. #13391, #13590). Because the feature is safely hidden behind a build flag, I figure its not harm in just keeping v9 up to date with directory sharing changes while we're still supporting it. That way, as we come up with non-directory-sharing improvements that nevertheless interact with these fundamental changes, we can still update branch/v9 with them with relative ease.

@zmb3
Copy link
Collaborator

zmb3 commented Jun 30, 2022

Hmmm, I'm not sure I want to backport all of this to v9. I'm especially concerned about putting the new role options in v9.

I agree that some of the more fundamental changes can be included if it makes backports easier for non-directory sharing features. I just wonder if there's a way to do this without pulling in everything.

@ibeckermayer
Copy link
Contributor Author

Hmmm, I'm not sure I want to backport all of this to v9. I'm especially concerned about putting the new role options in v9.

I agree that some of the more fundamental changes can be included if it makes backports easier for non-directory sharing features. I just wonder if there's a way to do this without pulling in everything.

My thinking was that the new role was fine to include -- it will just take the default value (directory_sharing: true), but that doesn't matter because the actual directory sharing feature switch is here

AllowDirectorySharing: authCtx.Checker.DesktopDirectorySharing() && allowDirectorySharing(),

which is still modulated by the directory_sharing build flag (represented by && allowDirectorySharing()). But I'm not an expert on our RBAC system and could be overlooking some problem.

This is essentially a lazy "kitchen sink" approach, just put all of our changes in v9 and rely on the build flag to protect us, rather than trying to pick out which ones will be structurally important one by one as they come in. That said, the RBAC changes fall outside of the possibility of causing "fundamental changes to how the rdp client works", and I don't see that changing in the near future. So what if I just remove #12684?

@ibeckermayer ibeckermayer enabled auto-merge (squash) June 30, 2022 19:30
@espadolini espadolini removed their request for review July 1, 2022 12:23
@ibeckermayer
Copy link
Contributor Author

@zmb3 and I chatted in Slack and agree that the "kitchen sink" approach referenced in this comment is actually the right way to go.

PTAL when you have a moment @probakowski @LKozlowski @xacrimon.

@github-actions
Copy link

github-actions bot commented Jul 7, 2022

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

@github-actions
Copy link

github-actions bot commented Jul 8, 2022

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

@github-actions
Copy link

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

@github-actions
Copy link

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

@ibeckermayer ibeckermayer merged commit 2eb1ff1 into branch/v9 Jul 11, 2022
@github-actions
Copy link

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

@zmb3 zmb3 deleted the isaiah/backport-12530-12531-12405-12615-12684-branch/v9 branch September 9, 2022 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants