Skip to content
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

feat: optimise local diff #1334

Merged
merged 5 commits into from
May 10, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .env.test
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ export E2E_TEST_SECRET="test"
export E2E_TEST_GH_TOKEN="test"

# Database
export DB_URI="postgres://isomer:password@localhost:54321/isomercms_test"
export DB_URI="postgres://isomer:password@127.0.0.1:54321/isomercms_test"
export DB_MIN_POOL="1"
export DB_MAX_POOL="10"
export DB_ENABLE_LOGGING="true"
10 changes: 10 additions & 0 deletions src/fixtures/review.ts
Original file line number Diff line number Diff line change
@@ -47,6 +47,9 @@ export const MOCK_LATEST_LOG_ONE = {
body: "body",
author_name: "name",
author_email: "email",
diff: {
files: [{ file: MOCK_COMMIT_FILEPATH_ONE + MOCK_COMMIT_FILENAME_ONE }],
},
}

export const MOCK_LATEST_LOG_TWO = {
@@ -57,6 +60,13 @@ export const MOCK_LATEST_LOG_TWO = {
body: "body",
author_name: "name",
author_email: "email",
diff: {
files: [{ file: MOCK_COMMIT_FILEPATH_TWO + MOCK_COMMIT_FILENAME_TWO }],
},
}

export const MOCK_LATEST_LOGS = {
all: [MOCK_LATEST_LOG_ONE, MOCK_LATEST_LOG_TWO],
}

export const MOCK_FILENAME_TO_LATEST_LOG_MAP = {
61 changes: 25 additions & 36 deletions src/services/db/GitFileSystemService.ts
Original file line number Diff line number Diff line change
@@ -385,7 +385,7 @@ export default class GitFileSystemService {
}

/**
* Wrapper over `git diff --name-only` that also creates `master` branch if it does not exist.
* Wrapper over `git diff-tree -r --name-only master..staging` that also creates `master` branch if it does not exist.
*/
getFilesChanged(repoName: string): ResultAsync<string[], GitFileSystemError> {
return this.createLocalTrackingBranchIfNotExists(
@@ -396,7 +396,7 @@ export default class GitFileSystemService {
return ResultAsync.fromPromise(
this.git
.cwd({ path: `${efsVolPath}/${repoName}`, root: false })
.diff(["master..staging", "--name-only"]),
.raw(["diff-tree", "-r", "--name-only", "master..staging"]),
kishore03109 marked this conversation as resolved.
Show resolved Hide resolved
(error) => {
logger.error(
`Error when getting diff files between master and staging: ${error}, when trying to access ${efsVolPath}/${repoName}`
@@ -419,42 +419,31 @@ export default class GitFileSystemService {
})
}

/**
* Get latest commit for a file path on a branch (including deleted files)
*/
getLatestCommitOfPath(
repoName: string,
path: string,
branch = "staging"
): ResultAsync<DefaultLogFields & ListLogLine, GitFileSystemError> {
const efsVolPath = this.getEfsVolPathFromBranch(branch)
return ResultAsync.fromPromise(
this.git
.cwd({ path: `${efsVolPath}/${repoName}`, root: false })
// -1 to return latest commit only, -- to get logs even for deleted files
.log(["-1", branch, "--", path]),
(error) => {
logger.error(
`Error when getting latest commit for "${path}" on "${branch}": ${error}, when trying to access ${efsVolPath}/${repoName}`
)

if (error instanceof GitError) {
return new GitFileSystemError(
"Unable to retrieve latest log info from disk"
getCommitsBetweenMasterAndStaging(
repoName: string
): ResultAsync<LogResult, GitFileSystemError> {
return this.createLocalTrackingBranchIfNotExists(
repoName,
"master"
).andThen(() => {
const efsVolPath = this.getEfsVolPathFromBranch("staging")
return ResultAsync.fromPromise(
this.git
.cwd({ path: `${efsVolPath}/${repoName}`, root: false })
.log(["master..staging", "--name-only"]),
(error) => {
logger.error(
`Error when getting commits between master and staging: ${error}, when trying to access ${efsVolPath}/${repoName}`
)
}

return new GitFileSystemError("An unknown error occurred")
}
).andThen((logs) => {
if (logs.latest === null) {
return errAsync(
new GitFileSystemError(
`No commit was found for "${path}" on "${branch}"`
)
)
}
return okAsync(logs.latest)
if (error instanceof GitError) {
return new GitFileSystemError(
"Unable to retrieve git logs info from disk"
)
}
return new GitFileSystemError("An unknown error occurred")
}
)
})
}

4 changes: 2 additions & 2 deletions src/services/db/RepoService.ts
Original file line number Diff line number Diff line change
@@ -58,8 +58,8 @@ export default class RepoService extends GitHubService {
return this.gitFileSystemService.getFilesChanged(siteName)
}

getLatestLocalCommitOfPath(repoName: string, path: string) {
return this.gitFileSystemService.getLatestCommitOfPath(repoName, path)
getCommitsBetweenMasterAndStaging(siteName: string) {
return this.gitFileSystemService.getCommitsBetweenMasterAndStaging(siteName)
}

getCommitDiff(siteName: string, base?: string, head?: string) {
83 changes: 37 additions & 46 deletions src/services/db/__tests__/GitFileSystemService.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import fs, { Stats } from "fs"

import mockFs from "mock-fs"
import { okAsync } from "neverthrow"
import { errAsync, okAsync } from "neverthrow"
import { GitError, SimpleGit } from "simple-git"

import config from "@config/config"
@@ -753,7 +753,7 @@ describe("GitFileSystemService", () => {

it("should return the files changed and defensively try creating local branches", async () => {
MockSimpleGit.cwd.mockReturnValueOnce({
diff: jest
raw: jest
.fn()
.mockResolvedValueOnce("fake-dir/fake-file\nanother-fake-file\n"),
})
@@ -768,7 +768,7 @@ describe("GitFileSystemService", () => {

it("should return GitFileSystemError if an error occurred when getting the git diff", async () => {
MockSimpleGit.cwd.mockReturnValueOnce({
diff: jest.fn().mockRejectedValueOnce(new GitError()),
raw: jest.fn().mockRejectedValueOnce(new GitError()),
})

const actual = await GitFileSystemService.getFilesChanged("fake-repo")
@@ -777,49 +777,6 @@ describe("GitFileSystemService", () => {
})
})

describe("getLatestLocalCommitOfPath", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm should add a getCommitsBetweenMasterAndStaging test case hor

it("should return the latest commit for a valid path", async () => {
MockSimpleGit.cwd.mockReturnValueOnce({
log: jest.fn().mockResolvedValueOnce({
latest: MOCK_LATEST_LOG_ONE,
}),
})

const result = await GitFileSystemService.getLatestCommitOfPath(
"fake-repo",
"fake-dir/fake-file"
)

expect(result._unsafeUnwrap()).toEqual(MOCK_LATEST_LOG_ONE)
})

it("should return GitFileSystemError if an error occurred when getting the git log", async () => {
MockSimpleGit.cwd.mockReturnValueOnce({
log: jest.fn().mockRejectedValueOnce(new GitError()),
})

const result = await GitFileSystemService.getLatestCommitOfPath(
"fake-repo",
"fake-dir/fake-file"
)

expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError)
})

it("should return GitFileSystemError if there were no commits found", async () => {
MockSimpleGit.cwd.mockReturnValueOnce({
log: jest.fn().mockResolvedValueOnce({ latest: null }),
})

const result = await GitFileSystemService.getLatestCommitOfPath(
"fake-repo",
"fake-dir/fake-file"
)

expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError)
})
})

describe("getGitLog", () => {
it("should return the Git log for a valid branch", async () => {
MockSimpleGit.cwd.mockReturnValueOnce({
@@ -3367,4 +3324,38 @@ describe("GitFileSystemService", () => {
expect(branch).not.toHaveBeenCalled()
})
})

describe("getCommitsBetweenMasterAndStaging", () => {
it("should get commits between staging and master branches", async () => {
jest
.spyOn(GitFileSystemService, "createLocalTrackingBranchIfNotExists")
.mockReturnValueOnce(okAsync(false))

const log = jest.fn().mockResolvedValueOnce({})

MockSimpleGit.cwd.mockReturnValueOnce({
log,
})

await GitFileSystemService.getCommitsBetweenMasterAndStaging("fake-repo")
expect(log).toHaveBeenCalledWith(["master..staging", "--name-only"])
})

it("should throw an error if there exists an error is getting log", async () => {
jest
.spyOn(GitFileSystemService, "createLocalTrackingBranchIfNotExists")
.mockReturnValueOnce(okAsync(false))

const log = jest.fn().mockRejectedValueOnce(new Error())

MockSimpleGit.cwd.mockReturnValueOnce({
log,
})
const result = await GitFileSystemService.getCommitsBetweenMasterAndStaging(
"fake-repo"
)

expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError)
})
})
})
Loading

Unchanged files with check annotations Beta

convict.addFormat({
name: "required-string",
validate: (val: any) => {

Check warning on line 5 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type

Check warning on line 5 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type
if (!val) throw new Error("value cannot be empty, null or undefined")
if (typeof val !== "string") throw new Error("value must be a string")
},
convict.addFormat({
name: "required-positive-number",
validate: (val: any) => {

Check warning on line 13 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type

Check warning on line 13 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type
if (val === null || val === undefined || val === "")
throw new Error("value cannot be empty, null or undefined")
if (typeof val !== "number") throw new Error("value must be a number")
},
coerce: (val: string) => {
const coercedVal = Number(val)
if (isNaN(coercedVal)) {

Check warning on line 20 in src/config/config.ts

GitHub Actions / lint

Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

Check warning on line 20 in src/config/config.ts

GitHub Actions / lint

Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan
throw new Error(
"value provided is not a positive number. please provide a valid positive number"
)
convict.addFormat({
name: "required-boolean",
validate: (val: any) => {

Check warning on line 34 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type

Check warning on line 34 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type
if (val === null || val === undefined)
throw new Error("value cannot be empty, null or undefined")
if (typeof val !== "boolean") throw new Error("value must be a boolean")
import { config } from "@config/config"
export enum JobStatus {

Check warning on line 5 in src/constants/constants.ts

GitHub Actions / lint

'JobStatus' is already declared in the upper scope on line 5 column 13

Check warning on line 5 in src/constants/constants.ts

GitHub Actions / lint

'JobStatus' is already declared in the upper scope on line 5 column 13
Ready = "READY", // Ready to run jobs
Running = "RUNNING", // A job is running
Failed = "FAILED", // A job has failed and recovery is needed
}
export enum SiteStatus {

Check warning on line 11 in src/constants/constants.ts

GitHub Actions / lint

'SiteStatus' is already declared in the upper scope on line 11 column 13

Check warning on line 11 in src/constants/constants.ts

GitHub Actions / lint

'SiteStatus' is already declared in the upper scope on line 11 column 13
Empty = "EMPTY", // A site record site is being initialized
Initialized = "INITIALIZED",
Launched = "LAUNCHED",
}
export enum RedirectionTypes {

Check warning on line 17 in src/constants/constants.ts

GitHub Actions / lint

'RedirectionTypes' is already declared in the upper scope on line 17 column 13

Check warning on line 17 in src/constants/constants.ts

GitHub Actions / lint

'RedirectionTypes' is already declared in the upper scope on line 17 column 13
CNAME = "CNAME",
A = "A",
}
export enum CollaboratorRoles {

Check warning on line 22 in src/constants/constants.ts

GitHub Actions / lint

'CollaboratorRoles' is already declared in the upper scope on line 22 column 13

Check warning on line 22 in src/constants/constants.ts

GitHub Actions / lint

'CollaboratorRoles' is already declared in the upper scope on line 22 column 13
Admin = "ADMIN",
Contributor = "CONTRIBUTOR",
IsomerAdmin = "ISOMERADMIN",
CollaboratorRoles.IsomerAdmin
>
export enum ReviewRequestStatus {

Check warning on line 33 in src/constants/constants.ts

GitHub Actions / lint

'ReviewRequestStatus' is already declared in the upper scope on line 33 column 13

Check warning on line 33 in src/constants/constants.ts

GitHub Actions / lint

'ReviewRequestStatus' is already declared in the upper scope on line 33 column 13
Approved = "APPROVED",
Open = "OPEN",
Merged = "MERGED",
export const FEATURE_FLAGS = {

Check warning on line 1 in src/constants/featureFlags.ts

GitHub Actions / lint

Prefer default export on a file with single export

Check warning on line 1 in src/constants/featureFlags.ts

GitHub Actions / lint

Prefer default export on a file with single export
IS_BUILD_TIMES_REDUCTION_ENABLED: "is_build_times_reduction_enabled",
IS_GGS_ENABLED: "is_ggs_enabled",
IS_SHOW_STAGING_BUILD_STATUS_ENABLED: "is_show_staging_build_status_enabled",