Skip to content

Create gRPC service to be used by the CLI #855

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

Merged
merged 3 commits into from
May 3, 2025

Conversation

adombeck
Copy link
Contributor

The current state of #782 adds the gRPC methods used by the CLI to the NSS service. They don't belong there, so this PR adds a new gRPC service which provides methods intended for use by the CLI.

UDENG-6492

@adombeck adombeck marked this pull request as ready for review March 21, 2025 12:45
@adombeck adombeck requested a review from a team as a code owner March 21, 2025 12:45
@adombeck adombeck requested a review from 3v1n0 March 24, 2025 10:16
@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 7 times, most recently from 371a5e8 to 5f50f59 Compare April 1, 2025 12:48
@adombeck adombeck requested review from 3v1n0 and denisonbarbosa April 1, 2025 13:01
@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch from 5f50f59 to 06df31f Compare April 1, 2025 13:41
@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 92.59259% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.50%. Comparing base (59be8dd) to head (62ffae0).

Files with missing lines Patch % Lines
internal/services/user/user.go 92.95% 5 Missing ⚠️
internal/services/permissions.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #855      +/-   ##
==========================================
- Coverage   85.50%   85.50%   -0.01%     
==========================================
  Files          82       82              
  Lines        5707     5690      -17     
  Branches      109      109              
==========================================
- Hits         4880     4865      -15     
+ Misses        772      770       -2     
  Partials       55       55              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Some suggestions that I think will make the changes better (I didn't add the same comment on every single crate of the module, but some apply to all three)

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 4 times, most recently from 9ddf755 to 0fdb6bc Compare April 8, 2025 13:25
@adombeck adombeck marked this pull request as draft April 9, 2025 10:21
@adombeck
Copy link
Contributor Author

adombeck commented Apr 9, 2025

Converting to draft until I fixed the tests

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 2 times, most recently from 8c14998 to 4b6c6fb Compare April 10, 2025 07:32
@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 2 times, most recently from e2cf75f to aaf77bf Compare April 10, 2025 12:28
@adombeck adombeck marked this pull request as ready for review April 10, 2025 13:38
@adombeck adombeck requested a review from denisonbarbosa April 11, 2025 09:29
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice work! Once @3v1n0 is also happy, feel free to merge.

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 2 times, most recently from 61b1d8d to 69dc5ec Compare April 11, 2025 22:08
@shiv-tyagi
Copy link
Contributor

Thanks @adombeck for the awesome work.

@shiv-tyagi
Copy link
Contributor

Can we get this merged soon? :)
I want to finish #782 and get it merged.

@shiv-tyagi
Copy link
Contributor

Can we get this merged soon? :)

Any update @adombeck?

@adombeck
Copy link
Contributor Author

@shiv-tyagi Sorry, we had other priorities the last two weeks. I hope we will get this merged next week!

@shiv-tyagi
Copy link
Contributor

@shiv-tyagi Sorry, we had other priorities the last two weeks. I hope we will get this merged next week!

No worries. Looking forward :-)

@adombeck adombeck requested a review from Copilot April 29, 2025 13:09
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates CLI-specific gRPC methods from the NSS service into a dedicated UserService. The key changes include renaming and refactoring of service packages and method calls, removal of the old NSS service implementation, and updates to gRPC proto definitions and client registrations.

Reviewed Changes

Copilot reviewed 36 out of 46 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/services/user/permissions.go Renamed package from "nss" to "user" to reflect the new service context.
internal/services/permissions.go Updated global access check to use userService instead of nssService.
internal/services/nss/nss.go Removed the entire NSS service implementation that is no longer needed.
internal/services/manager_test.go Updated tests to work with the new UserService client instead of the deprecated NSS client.
internal/services/manager.go Replaced references to nssService with userService, including gRPC registration changes.
internal/proto/authd/authd_grpc.pb.go Updated gRPC service definitions and client implementations from NSS to UserService.
internal/proto/authd/authd.proto Removed the NSS service definitions and added UserService definitions with updated message types.
Files not reviewed (10)
  • internal/services/nss/testdata/golden/TestGetGroupEntries/Return_all_groups: Language not supported
  • internal/services/nss/testdata/golden/TestGetGroupEntries/Return_no_groups: Language not supported
  • internal/services/nss/testdata/golden/TestGetShadowByName/Return_existing_user: Language not supported
  • internal/services/nss/testdata/golden/TestGetShadowEntries/Return_all_users: Language not supported
  • internal/services/nss/testdata/golden/TestGetShadowEntries/Return_no_users: Language not supported
  • internal/services/testdata/TestRegisterGRPCServices/golden: Language not supported
  • internal/services/testdata/golden/TestRegisterGRPCServices: Language not supported
  • internal/services/user/testdata/golden/TestGetGroupByID/Return_existing_group: Language not supported
  • internal/services/user/testdata/golden/TestGetGroupByName/Return_existing_group: Language not supported
  • internal/services/user/testdata/golden/TestGetUserByID/Return_existing_user: Language not supported

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch 2 times, most recently from f472873 to 82a3b47 Compare May 2, 2025 11:53
@adombeck
Copy link
Contributor Author

adombeck commented May 2, 2025

I rebased on main and resolved the conflicts

@adombeck
Copy link
Contributor Author

adombeck commented May 2, 2025

@3v1n0 anything still missing for your approval? Do you want to take another look?

@adombeck adombeck force-pushed the UDENG-6492-restructure-nss-service branch from 82a3b47 to 62ffae0 Compare May 2, 2025 16:38
@adombeck adombeck merged commit 110afe8 into main May 3, 2025
16 checks passed
@adombeck adombeck deleted the UDENG-6492-restructure-nss-service branch May 3, 2025 16:48
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.

6 participants