chore: Trying Augment to break down LdapUtils responsibilities#290
chore: Trying Augment to break down LdapUtils responsibilities#290definitelynotagoblin wants to merge 1 commit intov4from
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // Pass 'this' so that virtual-method overrides (e.g. via Moq) are respected. | ||
| _wkpService = new WellKnownPrincipalService(this, _domainControllerRegistry, _log); | ||
| _principalResolver = new PrincipalResolver(this, _wkpService, _domainControllerRegistry, _ldapConfig, _log, _metric); | ||
| _hostResolver = new HostResolver(_principalResolver, _portScanner, _nativeMethods, _log); |
There was a problem hiding this comment.
nit (non-blocking): the hostResolver is set to readonly, and is not updated in reset, yet it is reliant on the _principalResolver which is reset. After a reset the LdapUtils._principalResolver and the _hostResolver._principalResolvers are different instances. This may cause some cache misses on _distinguishedNameCache down the line.
| return true; | ||
| } | ||
|
|
||
| var resDomain = await _utils.GetDomainNameFromSid(domainName) is (false, var tempDomain) |
There was a problem hiding this comment.
question (non-blocking): augment flagged this as an existing bug? should the tempDomain be used if _utils.GetDomainNameFromSid is (false, _)?
| _dcRegistry.Add(domainControllerSID); | ||
| } | ||
|
|
||
| private IDirectoryObject CreateDirectoryEntry(string path) { |
There was a problem hiding this comment.
question (non-blocking): this function is duplicated in LdapUtils, should it instead be exposed through ILdapUtils and called from PrincipalResolver?
2 participants
Description
Motivation and Context
This PR addresses: [GitHub issue or Jira ticket number]
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: