Skip to content

Conversation

@kalanchan
Copy link
Contributor

part of CORE-1195: SAMS: remove DeleterUser endpoint from clientsv1, and user management RPC, and addressing @unknwon's comment

We initially implemented the DeleteUser RPC to handle all CRUD user management actions in the Enterprise Portal, but since then we have decided not to implement any deletion functionality in Enterprise Portal. To clean up the UsersService API, removing this to avoid managing 2 separate DeleteUser implementations.

Thanks to DeepSearch, this should be safe to remove as there are no downstream calls to the DeleteUser RPC

Test plan

CI

@kalanchan kalanchan requested a review from unknwon July 18, 2025 19:22
@kalanchan kalanchan requested a review from a team as a code owner July 18, 2025 19:22
@unknwon
Copy link
Contributor

unknwon commented Jul 18, 2025

Thanks for cleaning things up, but you sure you actually read the DeepSearch thread you linked? It clearly says there is one downstream call site: https://sourcegraph.sourcegraph.app/search?q=context:global+repo:%5Egithub%5C.com/sourcegraph/sourcegraph%24+users().DeleteUser&patternType=literal

We need to remove that call site first.

Copy link
Contributor

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

Request changes because we need to remove the call site that uses this method.

@kalanchan
Copy link
Contributor Author

kalanchan commented Jul 18, 2025

Thanks for cleaning things up, but you sure you actually read the DeepSearch thread you linked? It clearly says there is one downstream call site: https://sourcegraph.sourcegraph.app/search?q=context:global+repo:%5Egithub%5C.com/sourcegraph/sourcegraph%24+users().DeleteUser&patternType=literal

We need to remove that call site first.

@unknwon it's technically not being used though, there aren't any handlers calling store.DeleteSAMSUser(). I was going to remove that in a follow up after this.

Do you still recommend deleting that first before merging this?

@unknwon
Copy link
Contributor

unknwon commented Jul 18, 2025

Do you still recommend deleting that first before merging this?

Yes. BTW I think that's a requirement, not a recommendation, FWIW.

@kalanchan
Copy link
Contributor Author

kalanchan commented Jul 18, 2025

Do you still recommend deleting that first before merging this?

Yes. BTW I think that's a requirement, not a recommendation, FWIW.

fine! understood, will do 🫡

@kalanchan kalanchan requested a review from unknwon July 31, 2025 18:23
@kalanchan
Copy link
Contributor Author

joe please stamp

@kalanchan
Copy link
Contributor Author

@unknwon callsites have been removed in #6809, hurry up and please stamp!

@unknwon
Copy link
Contributor

unknwon commented Jul 31, 2025

@unknwon callsites have been removed in #6809, hurry up and please stamp!

I'm regret to inform you that your auto-stamp status has not been earned.

Copy link
Contributor

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

🚀

@kalanchan
Copy link
Contributor Author

@unknwon callsites have been removed in #6809, hurry up and please stamp!

I'm regret to inform you that your auto-stamp status has not been earned.

How do I earn the auto-stamp status? can I be placed in a program?

@kalanchan kalanchan merged commit 2e07940 into main Jul 31, 2025
2 checks passed
@kalanchan kalanchan deleted the kalan/remove-delete-user-rpc branch July 31, 2025 19:11
@unknwon
Copy link
Contributor

unknwon commented Jul 31, 2025

@unknwon callsites have been removed in #6809, hurry up and please stamp!

I'm regret to inform you that your auto-stamp status has not been earned.

How do I earn the auto-stamp status? can I be placed in a program?

First of all, do all of the follow ups.

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.

3 participants