Conversation
LalitDeore
commented
Mar 19, 2026
LalitDeore
commented
- While deleting app delete app related files and cloud function
- Run all app operations when app is created in health api instead of stopping it on failure.
|
spooky PR |
| // set the body | ||
| req.Body = ioutil.NopCloser(bytes.NewBuffer(reqBodyJson)) | ||
| reqBody := map[string]string{"execution_id": executionData.Id, "authorization": executionData.Authorization} | ||
| reqBodyJson, _ := json.Marshal(reqBody) |
There was a problem hiding this comment.
don't discard the error, i want us to track these.
There was a problem hiding this comment.
this is request body not response body. We are logging every error. Will verify again in case we are not logging erros :)
| if err != nil { | ||
| log.Printf("[ERROR] Failed creating HTTP for app delete request: %s", err) | ||
| return appHealth, err | ||
| delClient := &http.Client{Timeout: 60 * time.Second} |
There was a problem hiding this comment.
you sure you want this to stay to 60 seconds? the rest are bumped to 120
There was a problem hiding this comment.
Yes, delete app api triggers cloud resource deletion in background. It removes the app from the datastore and returns the response, which is fast as compared to other. We should optimize other apis in same way :)
There was a problem hiding this comment.
cool, i trust you on this then
| _, err = cloudfunctions.NewProjectsLocationsFunctionsService(service).Delete(functionPath).Do() | ||
| if err != nil { | ||
| // 404 means the function was never deployed or was already deleted — not an error. | ||
| if strings.Contains(err.Error(), "404") || strings.Contains(strings.ToLower(err.Error()), "not found") { |
There was a problem hiding this comment.
i hate the way we are doing error matching here, this works for now, but if we could do status code checking through the objects directly, things would feel better.
There was a problem hiding this comment.
actually, figure out a way to do this cleanly in this iteration itself.
a quick search presents:
var apiErr *googleapi.Error
if errors.As(err, &apiErr) && apiErr.Code == 404 {
let's clean this codebase :)
| // Delete extra_specs/{app.ID} — used when app data is too large for datastore. | ||
| extraSpecsPath := fmt.Sprintf("extra_specs/%s", app.ID) | ||
| if delErr := bucket.Object(extraSpecsPath).Delete(ctx); delErr != nil { | ||
| if !strings.Contains(delErr.Error(), "doesn't exist") && !strings.Contains(delErr.Error(), "no such object") { |
There was a problem hiding this comment.
Why not use if !errors.Is(delErr, storage.ErrObjectNotExist) { instead? remember, we are trying to make the health code easier to look at as well.
| if len(app.ReferenceUrl) > 0 && project.Environment == "cloud" { | ||
| appCopy := *app | ||
| go func() { | ||
| if err := deleteAppCloudResources(context.Background(), &appCopy); err != nil { |
There was a problem hiding this comment.
can we add a timeout for this? feel free to add retries + logging for resilience.
There was a problem hiding this comment.
Why do we need a timeout? These GCP operations run in the background. The API only triggers the function, starts the process, and deletes the app from the datastore. The Cloud Function and app bucket files continue to be deleted in the background.
There was a problem hiding this comment.
i wouldn't want to trust GCP. timeouts and retries make the integration more graceful for us.
| // names contain the MD5 hash — handles case differences between identifier | ||
| // (mixed case, used for filenames) and functionName (always lowercase). | ||
| for _, prefix := range []string{"generated_cloudfunctions/", "generated_apps/"} { | ||
| it := bucket.Objects(ctx, &storage.Query{Prefix: prefix}) |
There was a problem hiding this comment.
you're going to end up iterating through a lot. can you not use a more specific prefix?
something like app name with dashes stripped out?
There was a problem hiding this comment.
true. Let me check if there is any better way to do this.
There was a problem hiding this comment.
just see how we format app names. would make your life easier.
| @@ -385,62 +390,42 @@ func RunOpsAppHealthCheck(apiKey string, orgId string) (AppHealth, error) { | |||
|
|
|||
| executeBodyJSON, err := json.Marshal(executeBody) | |||
There was a problem hiding this comment.
this becomes very nested very soon. handle it in a clean helper function pls
| @@ -1567,148 +1574,144 @@ func RunOpsAppUpload(apiKey string, orgId string) (AppHealth, error) { | |||
|
|
|||
| executeBodyJSON, err := json.Marshal(executeBody) | |||
There was a problem hiding this comment.
use a similar helper function if possible. this gets confusing.
2 participants
