chore(quickie): delete quickie for gh

[kishore03109]

Problem

Actual removal of the no longer needed GithubCommitService file to reduce complexity + fix test cases.

[kishore03109]

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]

nit: wrong typing - remove branchName! could we also stricten fileSha to string?

[kishore03109]

good catch, removed

[seaerchin]

only looked at code changes and skimmed tests - not approving first because of sanity checks. code wise lgtm

[seaerchin]

i might be lacking context here but it seems abit weird to me to see branchName allowed as an argument here when we're explicitly removing it elsewhere (cos we know the branch should be STAGING_BRANCH) - is this because there are some call sites that need the branch to be different? (a merge to master perhaps)

[kishore03109]

Yes, this is the case! Specifically we have a RepoManagementService that calls on this branch name, and thus this API should be preserved

[seaerchin]

thanks for checking - seems like we (isomer admins) use it to reset branches so it should still be staging. that being said, it's ok to leave as-is cos this is hard to tell for sure + the added flexibility of resetting a diff branch might be useful for staging/staging-lite/master

[kishore03109]

@seaerchin

just a sanity check (feel free to mark as resolved) - all call sites either ignore (default to STAGING_BRANCH) or pass in STAGING_BRANCH explicitly right? otherwise there'll be a regression

Agreed, looked through all the call sites and found one pending getTree that still maintains the branchName as a arg but actually we dont need it already.

For the rest, the way i check is if the fun exists in RepoService, then make sure the call are compatible.
There exists some calls of func A in RepoService that calls func B in GithubService by using super (eg. readMediaFile. For those i check if the call sites are compatible.
^ reason for verbalising this is cuz yes quite a bit of code change, but i think this is sufficient to prevent regression, thoughts?

[seaerchin]

lgtm

[kishore03109]

Merge activity

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