-
Notifications
You must be signed in to change notification settings - Fork 120
Undeploy an API without having to delete the Host #1532
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
Changes from 5 commits
ad935a1
7240530
8514460
32b505d
d174427
a77f5e1
18d5334
cf6901f
01f1d76
96d18c4
d82da83
28b7de9
58b4de1
a953b2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,7 @@ pub trait ApiDeploymentService<AuthCtx, Namespace> { | |
async fn undeploy( | ||
&self, | ||
deployment: &ApiDeploymentRequest<Namespace>, | ||
auth_ctx: &AuthCtx, | ||
) -> Result<(), ApiDeploymentError<Namespace>>; | ||
|
||
// Example: A newer version of API definition is in dev site, and older version of the same definition-id is in prod site. | ||
|
@@ -419,16 +420,19 @@ impl<Namespace: GolemNamespace, AuthCtx: GolemAuthCtx> ApiDeploymentService<Auth | |
async fn undeploy( | ||
&self, | ||
deployment: &ApiDeploymentRequest<Namespace>, | ||
auth_ctx: &AuthCtx, | ||
) -> Result<(), ApiDeploymentError<Namespace>> { | ||
info!(namespace = %deployment.namespace, "Undeploying API definitions"); | ||
// Existing deployment | ||
|
||
// 1. Get existing deployment records | ||
let existing_deployment_records = self | ||
.deployment_repo | ||
.get_by_site(&deployment.site.to_string()) | ||
.await?; | ||
|
||
let mut remove_deployment_records: Vec<ApiDeploymentRecord> = vec![]; | ||
|
||
// 2. Filter records that match the API definition keys to undeploy | ||
for deployment_record in existing_deployment_records { | ||
if deployment_record.namespace != deployment.namespace.to_string() | ||
|| deployment_record.subdomain != deployment.site.subdomain | ||
|
@@ -457,10 +461,32 @@ impl<Namespace: GolemNamespace, AuthCtx: GolemAuthCtx> ApiDeploymentService<Auth | |
} | ||
|
||
if !remove_deployment_records.is_empty() { | ||
// 3. Get the specific API definitions being undeployed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing component constraints during undeploy. There is already a test in service_test. Here we do two loops, one to filter by name, so there is no namespace conflict. And another loop to get the actual api definition, it is possible to do this in a single loop. |
||
let mut definitions_to_undeploy = Vec::new(); | ||
for key in &deployment.api_definition_keys { | ||
if let Some(definition) = self | ||
.definition_repo | ||
.get(&deployment.namespace.to_string(), &key.id.0, &key.version.0) | ||
.await? | ||
{ | ||
definitions_to_undeploy.push( | ||
CompiledHttpApiDefinition::try_from(definition).map_err(|e| { | ||
ApiDeploymentError::conversion_error("API definition record", e) | ||
})?, | ||
); | ||
} | ||
} | ||
|
||
// 4. Remove component constraints | ||
self.remove_component_constraints(definitions_to_undeploy, auth_ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope you have a test around these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test for checking if component constraints have been removed, no not yet, I will add one |
||
.await?; | ||
|
||
// 5. Delete deployment records | ||
self.deployment_repo | ||
.delete(remove_deployment_records.clone()) | ||
.await?; | ||
|
||
// 6. Set undeployed as draft | ||
self.set_undeployed_as_draft(remove_deployment_records) | ||
.await?; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,9 @@ use golem_service_base::api_tags::ApiTags; | |
use golem_service_base::auth::{DefaultNamespace, EmptyAuthCtx}; | ||
use golem_worker_service_base::api::ApiEndpointError; | ||
use golem_worker_service_base::api::{ApiDeployment, ApiDeploymentRequest}; | ||
use golem_worker_service_base::gateway_api_definition::ApiDefinitionId; | ||
use golem_worker_service_base::gateway_api_definition::{ApiDefinitionId, ApiVersion}; | ||
use golem_worker_service_base::gateway_api_deployment; | ||
use golem_worker_service_base::gateway_api_deployment::ApiSite; | ||
use golem_worker_service_base::gateway_api_deployment::ApiSiteString; | ||
use golem_worker_service_base::service::gateway::api_definition::ApiDefinitionIdWithVersion; | ||
use golem_worker_service_base::service::gateway::api_deployment::ApiDeploymentService; | ||
|
@@ -182,4 +183,102 @@ impl ApiDeploymentApi { | |
|
||
Ok(Json("API deployment deleted".to_string())) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the endpoint. /:site/:id/:version |
||
/// Undeploy a single API definition from a site | ||
/// | ||
/// Removes a specific API definition (by id and version) from a site without deleting the entire deployment. | ||
#[oai( | ||
path = "/:site/:id/:version", | ||
method = "delete", | ||
operation_id = "undeploy_api" | ||
)] | ||
async fn undeploy_api( | ||
&self, | ||
site: Path<String>, | ||
id: Path<String>, | ||
version: Path<String>, | ||
) -> Result<Json<String>, ApiEndpointError> { | ||
let record = recorded_http_api_request!( | ||
"undeploy_api", | ||
site = site.0.clone(), | ||
id = id.0.clone(), | ||
version = version.0.clone() | ||
); | ||
|
||
let response = self | ||
.undeploy_api_internal(site.0, id.0, version.0, &EmptyAuthCtx::default()) | ||
.instrument(record.span.clone()) | ||
.await; | ||
record.result(response) | ||
} | ||
|
||
async fn undeploy_api_internal( | ||
&self, | ||
site: String, | ||
id: String, | ||
version: String, | ||
auth_ctx: &EmptyAuthCtx, | ||
) -> Result<Json<String>, ApiEndpointError> { | ||
let namespace = DefaultNamespace::default(); | ||
let api_definition_key = ApiDefinitionIdWithVersion { | ||
id: ApiDefinitionId(id), | ||
version: ApiVersion(version), | ||
}; | ||
|
||
// Split the site string into host and subdomain parts | ||
let (host, subdomain) = if let Some(idx) = site.find('.') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unreliable. I don't think you can really split host and site like that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fix this, there should be a way to convert ApiSiteString to ApiSite already within golem and use that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding host and subdomain by splitting dots is unreliable and may not work in all cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is a way to convert ApiSiteString to ApiSite. I think it's good to avoid that need, if you see what I mean. |
||
let (subdomain, host) = site.split_at(idx); | ||
( | ||
host.trim_start_matches('.').to_string(), | ||
Some(subdomain.to_string()), | ||
) | ||
} else { | ||
(site, None) | ||
}; | ||
|
||
let api_site = ApiSite { host, subdomain }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think That to undeploy an API from a site, you really don't need to logically split domain and subdomain anymore. |
||
|
||
// Check if the site exists | ||
let site_exists = self | ||
.deployment_service | ||
.get_by_site(&ApiSiteString::from(&api_site)) | ||
.await? | ||
.is_some(); | ||
|
||
if !site_exists { | ||
return Err(ApiEndpointError::not_found(safe( | ||
"Site not found".to_string(), | ||
))); | ||
} | ||
|
||
// Check if the API definition exists | ||
let api_definition_exists = self | ||
.deployment_service | ||
.get_by_id(&namespace, Some(api_definition_key.id.clone())) | ||
.await? | ||
.iter() | ||
.any(|deployment| { | ||
deployment.api_definition_keys.iter().any(|key| { | ||
key.id == api_definition_key.id && key.version == api_definition_key.version | ||
}) | ||
}); | ||
|
||
if !api_definition_exists { | ||
return Err(ApiEndpointError::not_found(safe( | ||
"API definition not found".to_string(), | ||
))); | ||
} | ||
|
||
let api_deployment = gateway_api_deployment::ApiDeploymentRequest { | ||
namespace: namespace.clone(), | ||
api_definition_keys: vec![api_definition_key], | ||
site: api_site, | ||
}; | ||
|
||
self.deployment_service | ||
.undeploy(&api_deployment, auth_ctx) | ||
.await?; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please move this logic out of the route into the ApiDeploymentService. Try to only do simple data transformations here. Looks good otherwise, thanks! |
||
Ok(Json("API definition undeployed from site".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.
added undeploy to test-framework