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.

Sign up for GitHub

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

Fix: Checkout Issue in self hosted runner due to faulty submodule check-ins #1196

Merged
merged 6 commits into from
Apr 14, 2023
Merged

Fix: Checkout Issue in self hosted runner due to faulty submodule check-ins #1196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7f14f1d
Fix Self hosted runner issue wrt bad submodules - solution cleanup wo…
megamanics Mar 2, 2023
39cfb84
Fix format with npm run format output
megamanics Mar 2, 2023
d12863b
Add mock implementation for new function submoduleStatus
megamanics Mar 2, 2023
c8466ed
Merge branch 'main' into megamanics/main
Link- Mar 9, 2023
bb965fa
Add 2 test cases for submodule status.
sminnie Mar 19, 2023
6b84265
Codeql-Action Analyse revert v1 to v2
megamanics Apr 12, 2023
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
3 changes: 3 additions & 0 deletions __test__/git-auth-helper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,9 @@ async function setup(testName: string): Promise<void> {
return ''
}),
submoduleSync: jest.fn(),
submoduleStatus: jest.fn(async () => {
return true
}),
submoduleUpdate: jest.fn(),
tagExists: jest.fn(),
tryClean: jest.fn(),
Expand Down
62 changes: 62 additions & 0 deletions __test__/git-directory-helper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,65 @@ describe('git-directory-helper tests', () => {
expect(git.branchDelete).toHaveBeenCalledWith(false, 'local-branch-2')
})

const cleanWhenSubmoduleStatusIsFalse =
'cleans when submodule status is false'

it(cleanWhenSubmoduleStatusIsFalse, async () => {
// Arrange
await setup(cleanWhenSubmoduleStatusIsFalse)
await fs.promises.writeFile(path.join(repositoryPath, 'my-file'), '')

//mock bad submodule

const submoduleStatus = git.submoduleStatus as jest.Mock<any, any>
submoduleStatus.mockImplementation(async (remote: boolean) => {
return false
})

// Act
await gitDirectoryHelper.prepareExistingDirectory(
git,
repositoryPath,
repositoryUrl,
clean,
ref
)

// Assert
const files = await fs.promises.readdir(repositoryPath)
expect(files).toHaveLength(0)
expect(git.tryClean).toHaveBeenCalled()
})

const doesNotCleanWhenSubmoduleStatusIsTrue =
'does not clean when submodule status is true'

it(doesNotCleanWhenSubmoduleStatusIsTrue, async () => {
// Arrange
await setup(doesNotCleanWhenSubmoduleStatusIsTrue)
await fs.promises.writeFile(path.join(repositoryPath, 'my-file'), '')

const submoduleStatus = git.submoduleStatus as jest.Mock<any, any>
submoduleStatus.mockImplementation(async (remote: boolean) => {
return true
})

// Act
await gitDirectoryHelper.prepareExistingDirectory(
git,
repositoryPath,
repositoryUrl,
clean,
ref
)

// Assert

const files = await fs.promises.readdir(repositoryPath)
expect(files.sort()).toEqual(['.git', 'my-file'])
expect(git.tryClean).toHaveBeenCalled()
})

const removesLockFiles = 'removes lock files'
it(removesLockFiles, async () => {
// Arrange
Expand Down Expand Up @@ -423,6 +482,9 @@ async function setup(testName: string): Promise<void> {
submoduleForeach: jest.fn(),
submoduleSync: jest.fn(),
submoduleUpdate: jest.fn(),
submoduleStatus: jest.fn(async () => {
return true
}),
Link- marked this conversation as resolved.
Show resolved Hide resolved
tagExists: jest.fn(),
tryClean: jest.fn(async () => {
return true
Expand Down
12 changes: 12 additions & 0 deletions dist/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,13 @@ class GitCommandManager {
yield this.execGit(args);
});
}
submoduleStatus() {
return __awaiter(this, void 0, void 0, function* () {
const output = yield this.execGit(['submodule', 'status'], true);
core.debug(output.stdout);
return output.exitCode === 0;
});
}
tagExists(pattern) {
return __awaiter(this, void 0, void 0, function* () {
const output = yield this.execGit(['tag', '--list', pattern]);
Expand Down Expand Up @@ -1023,6 +1030,11 @@ function prepareExistingDirectory(git, repositoryPath, repositoryUrl, clean, ref
}
}
core.endGroup();
// Check for submodules and delete any existing files if submodules are present
if (!(yield git.submoduleStatus())) {
remove = true;
core.info('Bad Submodules found, removing existing files');
}
// Clean
if (clean) {
core.startGroup('Cleaning the repository');
Expand Down
7 changes: 7 additions & 0 deletions src/git-command-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export interface IGitCommandManager {
submoduleForeach(command: string, recursive: boolean): Promise<string>
submoduleSync(recursive: boolean): Promise<void>
submoduleUpdate(fetchDepth: number, recursive: boolean): Promise<void>
submoduleStatus(): Promise<boolean>
tagExists(pattern: string): Promise<boolean>
tryClean(): Promise<boolean>
tryConfigUnset(configKey: string, globalConfig?: boolean): Promise<boolean>
Expand Down Expand Up @@ -357,6 +358,12 @@ class GitCommandManager {
await this.execGit(args)
}

async submoduleStatus(): Promise<boolean> {
const output = await this.execGit(['submodule', 'status'], true)
core.debug(output.stdout)
return output.exitCode === 0
}

Copy link
Contributor

Choose a reason for hiding this comment

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

for more info on the git submodule command, see: https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-status--cached--recursive--ltpathgt82308203

Copy link
Contributor

@jww3 jww3 Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I can tell (lots of googling and some experimentation with the git command line), git submodule status always returns an exit code of 0.

What am I missing here? I'd like to see for myself a case where git submodule status returns a non-zero exit code. Is there an easy way to demonstrate it?

In other words, what's the easiest way to "corrupt" a submodule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jww3 This can be recreated by cloning https://github.com/test-SKi/test-bad-module-clone & using checkout action with any self-hosted runner to see the failure.

Steps to recreate one scenario:

git checkout -b new
git clone git@github.com:test-SKi/submodule-repo.git
echo "recreate bad submodule issue" >>  README.md
git add .
git commit
git push --set-upstream origin new

async tagExists(pattern: string): Promise<boolean> {
const output = await this.execGit(['tag', '--list', pattern])
return !!output.stdout.trim()
Expand Down
6 changes: 6 additions & 0 deletions src/git-directory-helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ export async function prepareExistingDirectory(
}
core.endGroup()

// Check for submodules and delete any existing files if submodules are present
if (!(await git.submoduleStatus())) {
remove = true
core.info('Bad Submodules found, removing existing files')
}

// Clean
if (clean) {
core.startGroup('Cleaning the repository')
Expand Down