Skip to content

Conversation

@MichalFrends1
Copy link
Contributor

@MichalFrends1 MichalFrends1 commented Feb 3, 2026

Please review my changes :)

Review Checklist

  • Task version updated (x.x.0)
  • CHANGELOG.md updated
  • Solution builds
  • Warnings resolved (if possible)
  • Typos resolved
  • Tests cover new code
  • Description how to run tests locally added to README.md (if needed)
  • All tests pass locally

Summary by CodeRabbit

  • Documentation
    • Improved Host parameter documentation across Frends.LDAP packages with clearer descriptions and updated example values for LDAP host configuration.
  • Chores
    • Bumped package versions for multiple Frends.LDAP components.
  • Style
    • Minor formatting and whitespace cleanups across implementations.

@coderabbitai
Copy link

coderabbitai bot commented Feb 3, 2026

Walkthrough

Clarified Host XML documentation across multiple Frends.LDAP task modules, updated example host values, bumped package versions, added changelog entries, and applied minor formatting/attribute removals. No public API signatures or runtime logic were changed.

Changes

Cohort / File(s) Summary
AddUserToGroups
Frends.LDAP.AddUserToGroups/CHANGELOG.md, Frends.LDAP.AddUserToGroups/Definitions/Connection.cs, Frends.LDAP.AddUserToGroups/Frends.LDAP.AddUserToGroups.csproj
Clarified Host XML docs requiring DC FQDN in same domain, updated example to dc1.emea.company.com, bumped version to 2.1.0.
CreateUser
Frends.LDAP.CreateUser/CHANGELOG.md, Frends.LDAP.CreateUser/Definitions/Connection.cs, Frends.LDAP.CreateUser/Frends.LDAP.CreateUser.csproj
Enhanced Host documentation, updated example, bumped version to 1.2.0.
DeleteUser
Frends.LDAP.DeleteUser/CHANGELOG.md, Frends.LDAP.DeleteUser/Definitions/Connection.cs, Frends.LDAP.DeleteUser/Frends.LDAP.DeleteUser.csproj, Frends.LDAP.DeleteUser/DeleteUser.cs
Clarified Host docs to require DC FQDN in same domain; example updated; bumped version to 1.1.0. Minor formatting change in DeleteUser.cs.
RemoveUserFromGroups
Frends.LDAP.RemoveUserFromGroups/CHANGELOG.md, Frends.LDAP.RemoveUserFromGroups/Definitions/Connection.cs, Frends.LDAP.RemoveUserFromGroups/Frends.LDAP.RemoveUserFromGroups.csproj, Frends.LDAP.RemoveUserFromGroups/Definitions/Input.cs, Frends.LDAP.RemoveUserFromGroups/RemoveUserFromGroups.cs
Host docs clarified and example updated; bumped version to 1.1.0. Removed [DefaultValue] attribute from HandleLDAPError and applied minor whitespace formatting.
SearchObjects
Frends.LDAP.SearchObjects/CHANGELOG.md, Frends.LDAP.SearchObjects/Definitions/Connection.cs, Frends.LDAP.SearchObjects/Frends.LDAP.SearchObjects.csproj
Expanded Host docs to allow domain name, LDAP alias, or DC FQDN; example updated; bumped version to 4.2.0.
UpdateUser
Frends.LDAP.UpdateUser/CHANGELOG.md, Frends.LDAP.UpdateUser/Definitions/Connection.cs, Frends.LDAP.UpdateUser/Frends.LDAP.UpdateUser.csproj, Frends.LDAP.UpdateUser/UpdateUser.cs
Clarified Host documentation requiring DC FQDN, updated example, bumped version to 1.2.0. Minor whitespace adjustment in UpdateUser.cs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • MatteoDelOmbra
  • AndreasOlvebo-Frends

Poem

🐰 I hopped through docs with careful cheer,

Hostnames polished, examples clear.
DCs named in tidy line,
Versions bumped — everything fine.
A little rabbit dance of joy and gear.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'update summary for host param' is vague and generic, using non-descriptive terms that don't clearly convey what changes were made across the multiple LDAP task projects. Provide a more specific title that describes the main changes, such as 'Improve Host parameter documentation across LDAP tasks' or similar that reflects the actual scope of improvements.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch FSPES-64

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Frends.LDAP.RemoveUserFromGroups/Frends.LDAP.RemoveUserFromGroups/Frends.LDAP.RemoveUserFromGroups.csproj (1)

3-26: ⚠️ Potential issue | 🟠 Major

Fix whitespace formatting to unblock CI.
Pipeline reports WHITESPACE errors in this file; normalize indentation (tabs → spaces) or run the formatter.

🧹 Suggested indentation cleanup
-  <PropertyGroup>
-	<TargetFrameworks>net6.0</TargetFrameworks>
-	<Version>1.1.0</Version>
-	<Authors>Frends</Authors>
-	<Copyright>Frends</Copyright>
-	<Company>Frends</Company>
-	<Product>Frends</Product>
-	<PackageTags>Frends</PackageTags>
-	<PackageLicenseExpression>MIT</PackageLicenseExpression>
-	<GenerateDocumentationFile>true</GenerateDocumentationFile>
-	<Description>Remove a user from Active Directory groups.</Description>
-	<PackageProjectUrl>https://frends.com/</PackageProjectUrl>
-	<RepositoryUrl>https://github.com/FrendsPlatform/Frends.LDAP</RepositoryUrl>
-  </PropertyGroup>
-	
-  <ItemGroup>
-	  <None Include="FrendsTaskMetadata.json" Pack="true" PackagePath="/">
-		  <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
-	  </None>
-  </ItemGroup>
-	
-  <ItemGroup>
+  <PropertyGroup>
+    <TargetFrameworks>net6.0</TargetFrameworks>
+    <Version>1.1.0</Version>
+    <Authors>Frends</Authors>
+    <Copyright>Frends</Copyright>
+    <Company>Frends</Company>
+    <Product>Frends</Product>
+    <PackageTags>Frends</PackageTags>
+    <PackageLicenseExpression>MIT</PackageLicenseExpression>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <Description>Remove a user from Active Directory groups.</Description>
+    <PackageProjectUrl>https://frends.com/</PackageProjectUrl>
+    <RepositoryUrl>https://github.com/FrendsPlatform/Frends.LDAP</RepositoryUrl>
+  </PropertyGroup>
+
+  <ItemGroup>
+    <None Include="FrendsTaskMetadata.json" Pack="true" PackagePath="/">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
+  </ItemGroup>
+
+  <ItemGroup>
     <PackageReference Include="Novell.Directory.Ldap.NETStandard" Version="3.6.0" />
   </ItemGroup>
🤖 Fix all issues with AI agents
In `@Frends.LDAP.UpdateUser/Frends.LDAP.UpdateUser/Definitions/Connection.cs`:
- Around line 10-15: Update the XML documentation example for the Host property
on the Connection class to use a Domain Controller-style FQDN rather than a
generic hostname; locate the public property "Host" in Connection.cs and change
the <example> value to something like "dc1.emea.company.com" to match the
requirement that the host must resolve to a DC/FQDN.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
Frends.LDAP.DeleteUser/Frends.LDAP.DeleteUser/DeleteUser.cs (2)

10-11: ⚠️ Potential issue | 🔴 Critical

Make the task class static to satisfy FT0004.
The pipeline error indicates the task class must be static.

Proposed fix
-public class LDAP
+public static class LDAP

19-26: ⚠️ Potential issue | 🔴 Critical

Add required Options and CancellationToken parameters (FT0007) and honor cancellation.
The task signature is missing mandatory parameters, causing build failure.

Proposed fix
-    public static Result DeleteUser([PropertyTab] Input input, [PropertyTab] Connection connection)
+    public static Result DeleteUser(
+        [PropertyTab] Input input,
+        [PropertyTab] Connection connection,
+        [PropertyTab] Options options,
+        CancellationToken cancellationToken)
     {
+        cancellationToken.ThrowIfCancellationRequested();
         if (string.IsNullOrWhiteSpace(connection.Host) || string.IsNullOrWhiteSpace(connection.User) || string.IsNullOrWhiteSpace(connection.Password))
             throw new Exception("Connection parameters missing.");
+using System.Threading;

@MatteoDelOmbra MatteoDelOmbra merged commit e7b78da into main Feb 3, 2026
6 checks passed
@MatteoDelOmbra MatteoDelOmbra deleted the FSPES-64 branch February 3, 2026 11:29
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.

2 participants