8000 Bring back Microsoft.PowerShell.LocalAccounts module by iSazonov · Pull Request #12242 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Bring back Microsoft.PowerShell.LocalAccounts module #12242

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
8000

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
Closed
wants to merge 33 commits into from
Closed

Bring back Microsoft.PowerShell.LocalAccounts module #12242

wants to merge 33 commits into from

Conversation

iSazonov
Copy link
Collaborator
@iSazonov iSazonov commented Apr 1, 2020

PR Summary

Bring back Microsoft.PowerShell.LocalAccounts module.
Replace internal API with System.DirectoryServices.AccountManagement
Fix #2996

PR Context

#4302

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 1, 2020
@ghost ghost assigned anmenaga Apr 1, 2020
PoshChan
PoshChan reviewed Apr 1, 2020
View reviewed changes
PoshChan
PoshChan reviewed Apr 2, 2020
View reviewed changes
PoshChan
PoshChan reviewed Apr 2, 2020
View reviewed changes
PoshChan
PoshChan reviewed Apr 2, 2020
View reviewed changes
PoshChan
PoshChan 8000 reviewed Apr 2, 2020
View reviewed changes
@iSazonov
Copy link
Collaborator Author
iSazonov commented Apr 3, 2020 •
edited
Loading

@SteveL-MSFT @daxian-dbw @TravisEz13 Could you help me? Currently I port to AccountManagement API most of the code and fix tests but FindByIdentity() method works extremely slow (up to 100x(!) slower than PrincipalSearcher). I replace the method with PrincipalSearcher to search by name but I have to use the method to search by SID. Could you please point me better way? (I guess original code can resolve domain and Azure SIDs ...)

@iSazonov iSazonov mentioned this pull request Apr 6, 2020
Open
@iSazonov iSazonov mentioned this pull request Apr 11, 2020
Closed
@TravisEz13
Copy link
Member

I don't think this work should be part of PowerShell itself. The inbox module loads for me without issue.

@iSazonov
Copy link
Collaborator Author

Expectations are to get the module working on Unix.
The used API is potentially portable and provider based. A request to add PAM support is in .Net Runtime repository.

@ghost
Copy link
ghost commented May 29, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@ghost ghost added the Review - Needed The PR is being reviewed label May 29, 2020
TravisEz13
TravisEz13 requested changes May 29, 2020
View reviewed changes
Copy link
Member
@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

I don't think this work should be part of PowerShell itself. The inbox module loads for me without issue.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels May 29, 2020
@iSazonov
Copy link
Collaborator Author

Request to port AccountManagement API to Linux dotnet/runtime#37100

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 29, 2020
@TravisEz13 TravisEz13 added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 29, 2020
@TravisEz13
Copy link
Member

Why should this not be introduced as a module outside PowerShell first, then when it's working, brought into PowerShell?

@iSazonov
Copy link
Collaborator Author

@TravisEz13 It is minor details. Main question is how MSFT team sees a future of the module. If as open source and ported today is best time to do this - while .Net 5.0 is under development we could move to the public API for PowerShell 7.1. Then in .Net 6.0 timeline we could get portable API for next PowerShell 7.2 LTS.
If MSFT team want to keep the module as Windows-only and deprecated I could work on new AcoountManagement module (I already pulled example PR) with more modern design and features.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label May 31, 2020
@TravisEz13
Copy link
Member

The question was already answered, by the committee in a previous PR, that this should be a new module, published separately. Then we can answer the question about bringing back into PowerShell. That answer implies that the namespace should be separate.

@TravisEz13 TravisEz13 added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 1, 2020
@iSazonov
Copy link
Collaborator Author
iSazonov commented Jun 2, 2020

If MSFT team has clear understanding about future of LocalAccounts module I'd expect that MSFT team share this in new issue like:

  • LocalAccounts module is deprecated
  • new module should be designed
  • no concerns about backward compatibility
  • the new module should be community/personal, not Microsoft module
  • the new module should be published on PowerShellGet site, not in PowerShell/Module repository

Since my intention was to increase backward compatibility and to simplify migration to PowerShell Core I close the PR because I have no plans to develop a personal module.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 2, 2020
@iSazonov iSazonov closed this Jun 2, 2020
@iSazonov iSazonov mentioned this pull request Jun 2, 2020
Closed
12 tasks
@TravisEz13
Copy link
Member

The localaccounts module in windows continues to be supported, but otherwise, I think this is accurate. Basically, I think this falls under point 2 of Joey's slide here:

image

@iSazonov iSazonov mentioned this pull request Mar 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PoshChan PoshChan PoshChan left review comments

@TravisEz13 TravisEz13 TravisEz13 requested changes

@jshigetomi jshigetomi Awaiting requested review from jshigetomi jshigetomi will be requested when the pull request is marked ready for review jshigetomi is a code owner

Assignees

@anmenaga anmenaga

Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Get-LocalGroupMember - Failed to compare two elements in the array.
4 participants
@iSazonov @TravisEz13 @PoshChan @anmenaga
0