Skip to content
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

chore(services/user): migrate user-team-list proto op #1650

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ABCxFF
Copy link
Contributor

@ABCxFF ABCxFF commented Dec 18, 2024

Changes

Copy link
Contributor Author

ABCxFF commented Dec 18, 2024


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

cloudflare-workers-and-pages bot commented Dec 18, 2024

Deploying rivet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2c153f0
Status: ✅  Deploy successful!
Preview URL: https://7f0d6cff.rivet.pages.dev
Branch Preview URL: https://12-18-chore-services-user-mi-7ha2.rivet.pages.dev

View logs

@ABCxFF ABCxFF force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from 06909a8 to 4c41e8e Compare December 21, 2024 00:45
@ABCxFF ABCxFF force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from 4c41e8e to 075a092 Compare December 30, 2024 23:07
Copy link
Contributor

@MasterPtato MasterPtato left a comment

Choose a reason for hiding this comment

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

I know this is still in progress, just providing some initial feedback. Feel free to ignore any changes you've already made. Really good stuff all around.

I only have nits to add on top of the comments i left:

  • You can remove the leading :: from all user pkg paths (::user::ops::get -> user::ops::get)
  • Auto dereferencing should handle calls to .op in API services automatically, so you can change (*ctx) to just ctx
  • The migration files from api-identity were moved to the wrong location, the entire folder at packages/services/user-identity/db/user-identity should be moved to packages/services/user/db/user-identity
  • In the API layer we could change functions to accept an OperationCtx (new, from chirp-workflow) or a Ctx (from api-helper) instead of an OperationContext (old) which would allow us to get rid of compat calls and just do .op. This is low priority though and isn't required

team_ids: team_ids
.iter()
.map(|id| (*id).into())
.collect::<Vec<common::Uuid>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you can leave out the type in .collect statements if there is enough type info for the compiler to predict the type:

Suggested change
.collect::<Vec<common::Uuid>>()
.collect::<Vec<_>>()

or even

Suggested change
.collect::<Vec<common::Uuid>>()
.collect::<_>()

Comment on lines +47 to +57
chirp_workflow::compat::op(
&ctx,
::user::ops::identity::create::Input {
user_id: primary_user_id,
identity: backend::user_identity::Identity {
kind: Some(backend::user_identity::identity::Kind::Email(backend::user_identity::identity::Email {
email: util::faker::email()
}))
}
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Standalone services should be converted to use the new contexts as well, that way you dont have to use compat. See cluster-gc for an example. You'll have to change the ctx variable above to use StandaloneCtx

Comment on lines +47 to +57
chirp_workflow::compat::op(
&ctx,
::user::ops::identity::create::Input {
user_id: primary_user_id,
identity: backend::user_identity::Identity {
kind: Some(backend::user_identity::identity::Kind::Email(backend::user_identity::identity::Email {
email: util::faker::email()
}))
}
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment


#[derive(Debug)]
pub struct Output {
pub users: Vec<backend::user::User>
Copy link
Contributor

Choose a reason for hiding this comment

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

The User type should be converted from proto to rust. It looks like you already know the drill given the team_list op 👍

#[derive(Debug)]
pub struct Input {
pub user_id: Uuid,
pub identity: backend::user_identity::Identity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Type should be converted from proto to rust

#[derive(Debug)]
pub struct User {
pub user_id: Uuid,
pub identities: Vec<backend::user_identity::Identity>
Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment


#[derive(Debug)]
pub struct Output {
pub errors: Vec<common::ValidationError>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be converted to rust type

#[derive(Debug)]
pub struct Input {
pub user_id: Uuid,
pub client: backend::net::ClientInfo,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be converted to rust type


let res = ctx.op(::user::ops::token_create::Input {
user_id: user_id,
client: backend::net::ClientInfo {..Default::default()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
client: backend::net::ClientInfo {..Default::default()}
client: backend::net::ClientInfo::default()

@ABCxFF ABCxFF force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from e508246 to ec88581 Compare January 7, 2025 15:45
@ABCxFF ABCxFF force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from ec88581 to d8bc66a Compare January 14, 2025 22:47
@ABCxFF ABCxFF force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from d8bc66a to 6fb3d05 Compare January 22, 2025 03:35
@ABCxFF ABCxFF force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from 6fb3d05 to c1c6586 Compare January 22, 2025 21:10
@ABCxFF ABCxFF force-pushed the 12-18-chore_services_user_migrate_user_proto_ops branch from c1c6586 to 2c153f0 Compare January 22, 2025 22:19
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