feat(gitFileSystem): safer api #1046

kishore03109 · 2023-12-03T22:23:31Z

Problem

the presence of default arguments lead to sneaky bugs introduced due to caller unknowingly calling the wrong branch.
there is a scary instance whereby the lack of update to staging-lite could occur, but the update to staging does not, leading to issues downstream.

Closes [insert issue #]

Solution

to keep things simple, we make all the args compulsory.
make staging update first before updating staging lite.

kishore03109 · 2023-12-03T22:23:37Z

Current dependencies on/for this PR:

develop
- PR hotfix(repair-form): set remote url correctly #1048
  - PR feat(quickie): only gitfile should have quickie #1042
    - PR chore(quickie): delete quickie for gh #1043
      - PR test(githubService): add tests #1045
        
        PR feat(gitFileSystem): safer api #1046 👈
        
        PR test(gitCommitService): add test cases #1047
        
        PR fix(rollback handler): convert to ts for safety #1044

This stack of pull requests is managed by Graphite.

seaerchin

the changes are fine, albeit repetitive. similar work should probably be abstracted out as a function to avoid slight behavioural drift over time that creates bugs. this is a maintainability issue but it's not too bad for now.

otoh, we have a possibility of a silent failure, where we commit to EFS but fail the push to github. this could potentially be very confusing and we could fail fast here or attempt to handle the error properly. no pref, as long as the error itself is handled.

seaerchin · 2023-12-06T05:50:30Z

src/routes/formsg/formsgGGsRepair.ts

@@ -230,7 +230,7 @@ ${syncedRepos.map((repo) => `<li>${repo}</li>`)}

  doesRepoNeedClone(repoName: string): ResultAsync<true, false> {
    return this.gitFileSystemService
-      .isGitInitialized(repoName)
+      .isGitInitialized(repoName, true)


i'm assuming that this change was made because the method should only be used as part of the repair form?

this is also out of scope but omitting the argument (branchName or isStaging) and exporting this function might lead to people calling this for staging and wondering why it doesn't work.

we can either inline this directly or make it private (cos the false case can always be chained off an orElse) but ok with not doing this now because it's not a functional change and more of a good to have.

src/services/db/GitFileCommitService.ts

seaerchin · 2023-12-06T06:00:14Z

src/services/db/GitFileCommitService.ts

-        this.STAGING_LITE_BRANCH
-      )
-    }
+    this.pushToGithub(sessionData, shouldUpdateStagingLite)


this can fail silently - we should probably just chain off this. alternatively, what we can do is just chain off the results sequentially and throw an error at the end (the behaviour now), which makes things clearer to read.

also eliminates the issue of staging-lite updating but not staging cos if the initial call to staging fails, staging-lite won't even run

Hey this was an intentional trade off we made as a team not to await for calls for they are expensive. But I do air your concerns regarding silent failures, this is the cause of the divergence that we are seeing. Dont intend to tackle it here!

For info only + outside of scope of pr: I tried adding an alarm for this, but it was TOO noisy, there is prob a way to make this such that we only throw alarms for this silent pushes, but havnt had time to invetigate

we have a few different ways to tackle this tbh, just spitballing ideas - not an ask to implement.

separate out the push to github and let the router handle this. the router can always send response early then fail later (no cost to user) if the push to github fails.

because neverthrow is safe, we could just write a wrapper for retrying Errors<T>

just send an email on failure

we probably want to decide what categories of errors we care about - transient errors vs persistent errors; in terms of our usage, transient is probably outages <10 mins in length (we don't have that much pushes tbvh) and persistent >=10.

if transient, retry w/ exponential backoff will work well; if persistent, we probably want a notif + manual fix. to be honest, the divergence should arise only if we edit on github directly whilst there's a push failure so should be minimal but the problem is we got high barrier of entry to edit on efs (hence why we edit on github directly).

seaerchin · 2023-12-06T06:02:25Z

src/services/db/GitFileCommitService.ts

-        this.STAGING_LITE_BRANCH
-      )
-    }
+    this.pushToGithub(sessionData, shouldUpdateStagingLite)


same issue as above; there's also a repeated pattern - we could probably just separate out the steps as functions and chain them together.

seaerchin · 2023-12-06T06:05:33Z

src/services/db/GitFileCommitService.ts

this file has alot of the same pattern of do some change -> check the result of that change -> update staging lite -> update github -> throw error if any surface.

we could probably separate those out and make this whole file clearer. that being said, we don't have to do it now.

however, there is an issue where updates are allowed to silently fail at the push step. because our call returns a result, it won't throw by design and we need to check if it's recoverable or we should just fail fast (throw error).

src/services/db/GitFileSystemService.ts

kishore03109 · 2023-12-06T07:36:06Z

@seaerchin

the changes are fine, albeit repetitive. similar work should probably be abstracted out as a function to avoid slight behavioural drift over time that creates bugs. this is a maintainability issue but it's not too bad for now.

I am quite curious how you would have structured this instead (but not today kek release first), maybe i book time w u after i come back to learn from you

otoh, we have a possibility of a silent failure, where we commit to EFS but fail the push to github. this could potentially be very confusing and we could fail fast here or attempt to handle the error properly. no pref, as long as the error itself is handled.

I do think this is outside of the scope of this pr, since this is parity to what currently exists in production! thoughts?

seaerchin · 2023-12-06T07:47:21Z

I am quite curious how you would have structured this instead (but not today kek release first), maybe i book time w u after i come back to learn from you

ye sure

I do think this is outside of the scope of this pr, since this is parity to what currently exists in production! thoughts?

if there's no easy win here then sure, we can just go with this

seaerchin

lgtm if no quick way of feeding back failures to us.

do remove extra console log before merge.

kishore03109 · 2023-12-06T10:51:33Z

Merge activity

Dec 6, 5:51 AM: @kishore03109 started a stack merge that includes this pull request via Graphite.
Dec 6, 5:56 AM: Graphite rebased this pull request as part of a merge.
Dec 6, 5:57 AM: @kishore03109 merged this pull request with Graphite.

This was referenced Dec 3, 2023

feat(quickie): only gitfile should have quickie #1042

Merged

chore(quickie): delete quickie for gh #1043

Merged

fix(rollback handler): convert to ts for safety #1044

Merged

test(githubService): add tests #1045

Merged

kishore03109 mentioned this pull request Dec 3, 2023

test(gitCommitService): add test cases #1047

Merged

kishore03109 requested a review from a team December 3, 2023 22:54

kishore03109 marked this pull request as ready for review December 3, 2023 22:57

kishore03109 force-pushed the 12-04-test_githubService_add_tests branch from c8c6807 to 06a8feb Compare December 4, 2023 04:13

kishore03109 force-pushed the 12-04-feat_gitFileSystem_safer_api branch from 5757b98 to 2a826b8 Compare December 4, 2023 04:13

kishore03109 mentioned this pull request Dec 4, 2023

hotfix(repair-form): set remote url correctly #1048

Merged

kishore03109 force-pushed the 12-04-test_githubService_add_tests branch from 06a8feb to 18d81c8 Compare December 4, 2023 04:57

kishore03109 force-pushed the 12-04-feat_gitFileSystem_safer_api branch from 2a826b8 to 3050748 Compare December 4, 2023 04:57

kishore03109 force-pushed the 12-04-test_githubService_add_tests branch 2 times, most recently from 9c10830 to 437d36c Compare December 6, 2023 01:19

kishore03109 force-pushed the 12-04-feat_gitFileSystem_safer_api branch 3 times, most recently from ecc5966 to c2f88d4 Compare December 6, 2023 02:08

seaerchin reviewed Dec 6, 2023

View reviewed changes

kishore03109 requested a review from seaerchin December 6, 2023 07:36

seaerchin approved these changes Dec 6, 2023

View reviewed changes

kishore03109 force-pushed the 12-04-test_githubService_add_tests branch from 56c6bde to 19e412d Compare December 6, 2023 10:54

kishore03109 changed the base branch from 12-04-test_githubService_add_tests to develop December 6, 2023 10:55

kishore03109 added 4 commits December 6, 2023 10:56

feat(gitFileSystem): safer api

05f1bbe

Untitled commit

844aac7

Untitled commit

f64528e

Untitled commit

9f8bca5

Untitled commit

7d9bced

kishore03109 force-pushed the 12-04-feat_gitFileSystem_safer_api branch from 27d9fd6 to 7d9bced Compare December 6, 2023 10:56

kishore03109 merged commit 22f239c into develop Dec 6, 2023
8 checks passed

mergify bot deleted the 12-04-feat_gitFileSystem_safer_api branch December 6, 2023 10:57

This was referenced Dec 6, 2023

0.56.0 #1049

Merged

release(v0.56.0): merge to prod #1050

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gitFileSystem): safer api #1046

feat(gitFileSystem): safer api #1046

kishore03109 commented Dec 3, 2023 •

edited

Loading

kishore03109 commented Dec 3, 2023 •

edited

Loading

seaerchin left a comment

seaerchin Dec 6, 2023

seaerchin Dec 6, 2023

kishore03109 Dec 6, 2023

seaerchin Dec 6, 2023

seaerchin Dec 6, 2023

seaerchin Dec 6, 2023

kishore03109 commented Dec 6, 2023

seaerchin commented Dec 6, 2023 •

edited

Loading

seaerchin left a comment

kishore03109 commented Dec 6, 2023 •

edited

Loading

feat(gitFileSystem): safer api #1046

feat(gitFileSystem): safer api #1046

Conversation

kishore03109 commented Dec 3, 2023 • edited Loading

Problem

Solution

kishore03109 commented Dec 3, 2023 • edited Loading

seaerchin left a comment

Choose a reason for hiding this comment

seaerchin Dec 6, 2023

Choose a reason for hiding this comment

seaerchin Dec 6, 2023

Choose a reason for hiding this comment

kishore03109 Dec 6, 2023

Choose a reason for hiding this comment

seaerchin Dec 6, 2023

Choose a reason for hiding this comment

seaerchin Dec 6, 2023

Choose a reason for hiding this comment

seaerchin Dec 6, 2023

Choose a reason for hiding this comment

kishore03109 commented Dec 6, 2023

seaerchin commented Dec 6, 2023 • edited Loading

seaerchin left a comment

Choose a reason for hiding this comment

kishore03109 commented Dec 6, 2023 • edited Loading

Merge activity

kishore03109 commented Dec 3, 2023 •

edited

Loading

kishore03109 commented Dec 3, 2023 •

edited

Loading

seaerchin commented Dec 6, 2023 •

edited

Loading

kishore03109 commented Dec 6, 2023 •

edited

Loading