-
Notifications
You must be signed in to change notification settings - Fork 18
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
refactor: OrgId, WorkspaceId, SchemaName cleanup and refactor #379
Conversation
2d17790
to
3ec2e7f
Compare
} | ||
|
||
impl fmt::Display for HeadersEnum { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
match self { | ||
Self::ConfigVersion => write!(f, "x-config-version"), | ||
Self::TenantId => write!(f, "x-tenant"), | ||
Self::WorkspaceId => write!(f, "x-tenant"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something for later: should we start supporting x-workspace-id
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to support x-org-id
here, I had earlier added it but later removed, thinking it has some DB dependency for which I think I would need to update the list of enums in DB, if that is not the case then I can add it
also, we soon need to migrate x-tenant
to x-workspace
or x-workspace-id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
migration is difficult, we'll need to be backwards compatible for a bit. I need @mahatoankitkumar to weigh in on how this is being used
log::error!("Schema name not found, please check that the organisation id and workspace id are being properly sent"); | ||
actix_web::error::ErrorInternalServerError("Schema name not found, please check that the organisation id and workspace id are being properly sent") | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
saying schema_name not found and then asking for org id and workspace id is confusing, can we drop
Schema name not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
workspace_id: workspace_request.workspace_id.to_string(), | ||
organisation_id: workspace_request.organisation_id.to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mahatoankitkumar do you have to make any changes to our webhook receiver? Are you dependent on the tenant_id?
3ec2e7f
to
45f9f8e
Compare
@@ -462,24 +442,20 @@ pub async fn conclude( | |||
let url = format!("{}/default-config/{}", state.cac_host, key); | |||
|
|||
let headers = construct_request_headers(&[ | |||
("x-tenant", tenant_name.as_str()), | |||
("x-tenant", &workspace_request.workspace_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to call this x-tenant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this has not yet been migrated to something like x-workspace
or x-workspace-id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recap: This header is externally exposed, so we have to migrate those services as well.
45f9f8e
to
de62fa8
Compare
c2df318
to
8d10270
Compare
8d10270
to
e86bb30
Compare
fn get_workspace_id(&self) -> Option<WorkspaceId>; | ||
} | ||
|
||
impl ServiceRequestExt for ServiceRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLEAN!
Resolving the following issues
#353
#356
#361