GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Bring back Microsoft.PowerShell.LocalAccounts module by iSazonov · Pull Request #12242 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Apr 1, 2020

Copy link
Copy Markdown
Collaborator

PR Summary

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

PR Context

#4302

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • Breaking changes
    • None
    • OR
    • Experimental feature(s) needed
      • Experimental feature name(s):
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
  • Tooling
    • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
    • OR
    • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
      • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
      • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@iSazonov Ilya (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 Andrew (anmenaga) Apr 1, 2020
PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 1, 2020
View reviewed changes
Comment thread build.psm1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 2, 2020
View reviewed changes
Comment thread build.psm1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 2, 2020
View reviewed changes
Comment thread build.psm1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 2, 2020
View reviewed changes
Comment thread build.psm1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 2, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 3, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 3, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Apr 3, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

Steve Lee (@SteveL-MSFT) Dongbo Wang (@daxian-dbw) Travis Plunk (@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 ...)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 4, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 5, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 5, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 5, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 5, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 5, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 5, 2020
View reviewed changes
Comment thread assets/files.wxs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov Ilya (iSazonov) mentioned this pull request Apr 6, 2020
Principal.FindByIdentity() method is extremely slow dotnet/runtime#34598
Open
Ilya (iSazonov) added 12 commits April 8, 2020 21:43
@iSazonov
Bring back Microsoft.PowerShell.LocalAccounts module
fd73150
@iSazonov
Fix tests 1
c16b2eb
@iSazonov
Fix tests 2
2490fc3
@iSazonov
Fix tests 3
00cdcd1
@iSazonov
Fix test 4
71eadf6
@iSazonov
Fix packaging
60fd533
@iSazonov
Update resource strings
d6c0005
@iSazonov
Fix tests 5
0858182
@iSazonov
Fix test 6
e311a2a
@iSazonov
Fix test 7
11f592a
@iSazonov
Fix test 8 (CI)
4b956d1
@iSazonov
Perf: GetLocalUser
e273dbd
Ilya (iSazonov) added 7 commits April 8, 2020 21:46
@iSazonov
Remove old code from Sam.cs 2
1db4df7
@iSazonov
Remove Enabling enum
5f0f841
@iSazonov
Replace FindByIdentity()
9a81328
@iSazonov
Remove SamApi.cs
8bd2ec8
@iSazonov
CLeanup native
92afdb1
@iSazonov
Cleanup utils and exceptions
74ca4f9
@iSazonov
Perf: Rename-LocalUser
c918ed7
@iSazonov
Ilya (iSazonov) force-pushed the localaccounts branch from 7c50306 to c918ed7 Compare April 8, 2020 16:46
@iSazonov Ilya (iSazonov) mentioned this pull request Apr 11, 2020
Add-LocalGroupMember prefer to add domain account over local #12289
Closed
@TravisEz13

Travis Plunk (TravisEz13) commented May 21, 2020

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented May 22, 2020

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

Deleted user (ghost) commented May 29, 2020

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost ghost added the Review - Needed The PR is being reviewed label May 29, 2020
TravisEz13
Travis Plunk (TravisEz13) requested changes May 29, 2020
View reviewed changes

@TravisEz13 Travis Plunk (TravisEz13) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@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

Ilya (iSazonov) commented May 29, 2020

Copy link
Copy Markdown
Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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 Travis Plunk (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

Travis Plunk (TravisEz13) commented May 29, 2020

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented May 31, 2020

Copy link
Copy Markdown
Collaborator Author

Travis Plunk (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

Travis Plunk (TravisEz13) commented Jun 1, 2020

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TravisEz13 Travis Plunk (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

Ilya (iSazonov) commented Jun 2, 2020

Copy link
Copy Markdown
Collaborator Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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 Ilya (iSazonov) closed this Jun 2, 2020
@iSazonov Ilya (iSazonov) mentioned this pull request Jun 2, 2020
Add module AccountManagement #9926
Closed
12 tasks
@TravisEz13

Travis Plunk (TravisEz13) commented Jun 2, 2020

Copy link
Copy Markdown
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

orbanbalage reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov Ilya (iSazonov) mentioned this pull request Mar 18, 2023
Get-LocalGroupMember - Failed to compare two elements in the array. #2996
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 requested changes
@jshigetomi Justin Chung (jshigetomi) Awaiting requested review from jshigetomi jshigetomi will be requested when the pull request is marked ready for review jshigetomi is a code owner
Copilot code review Copilot
Copilot will automatically review once the pull request is marked ready for review
+1 more reviewer
@PoshChan PoshChan-Bot (PoshChan) PoshChan left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@anmenaga Andrew (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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@iSazonov @TravisEz13 @PoshChan @anmenaga
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.