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

get git sha on install #61

Merged
merged 5 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
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
6 changes: 3 additions & 3 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:

strategy:
matrix:
node-version: [10.x, 12.x]
node-version: [18.x, 20.x]

steps:
- uses: actions/checkout@v2
Expand All @@ -30,7 +30,7 @@ jobs:

strategy:
matrix:
node-version: [10.x, 12.x]
node-version: [18.x, 20.x]

steps:
- uses: actions/checkout@v2
Expand All @@ -51,7 +51,7 @@ jobs:

strategy:
matrix:
node-version: [10.x, 12.x]
node-version: [18.x, 20.x]

steps:
- uses: actions/checkout@v2
Expand Down
40 changes: 38 additions & 2 deletions report.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ function skipTraversal (rootPackage) {
}

function parentIsRoot (dependencyToReport) {
return dependencyToReport.parent.name === dependencyToReport.rootPackage.name &&
dependencyToReport.parent.version === dependencyToReport.rootPackage.version
return dependencyToReport?.parent?.name === dependencyToReport?.rootPackage?.name &&
dependencyToReport?.parent?.version === dependencyToReport?.rootPackage?.version
}

function isTopLevel (dependencyToReport) {
Expand Down Expand Up @@ -193,6 +193,22 @@ function processDependencyTreeOutput (resolve, reject) {
}
}

function processGitRevParseOutput (resolve, reject) {
return function (error, stdout, stderr) {
if (error && !stdout) {
return reject(new Error(`Scarf received an error from git rev-parse: ${error} | ${stderr}`))
}

const output = String(stdout).trim()

if (output.length > 0) {
return resolve(output)
} else {
return reject(new Error('Scarf did not receive usable output from git rev-parse'))
}
}
}

// packageJSONOverride: a test convenience to set a packageJSON explicitly.
// Leave empty to use the actual root package.json.
async function getDependencyInfo (packageJSONOverride) {
Expand Down Expand Up @@ -226,6 +242,18 @@ async function getDependencyInfo (packageJSONOverride) {
})
}

async function getGitShaFromRootPath () {
const promise = new Promise((resolve, reject) => {
exec(`cd ${rootPath} && git rev-parse HEAD`, { timeout: execTimeout, maxBuffer: 1024 * 1024 * 1024 }, processGitRevParseOutput(resolve, reject))
Copy link
Contributor

Choose a reason for hiding this comment

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

Going to the rootPath is only sometimes what we'd want. I think we should be going to the package importing scarf-js, ie the parentPath rather than the rootPath. This does handle Superset's specific use case but this will actually grab the wrong SHA in other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think then we should be probably only doing this on demand when the parent is the root, since you don't get git shas from node_modules installs

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I didn't realize that. Ok yeah then perhaps let's explicitly check for that and document that behavior in case we want to update that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i at least updated the function name that this only gets the root path git sha

})
try {
return await promise
} catch (e) {
logIfVerbose(e)
return undefined
}
}

async function reportPostInstall () {
const scarfApiToken = process.env.SCARF_API_TOKEN

Expand All @@ -235,6 +263,12 @@ async function reportPostInstall () {
return Promise.reject(new Error('No parent found, nothing to report'))
}

if (parentIsRoot(dependencyInfo) && allowTopLevel(dependencyInfo.rootPackage)) {
const gitSha = await getGitShaFromRootPath()
logIfVerbose(`Injecting sha to parent: ${gitSha}`)
dependencyInfo.parent.gitSha = gitSha
Copy link
Contributor

Choose a reason for hiding this comment

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

allowTopLevel on its own does not guarantee that parent == root. I think this logic would work if skipTraversal && allowTopLevel but we probably need to be a little more general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think then i should actually update this to make use of parentIsRoot and fix up parentIsRoot to be safe to use with optionals, realizing that the sha can only be retrieved from the actual root package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i updated the condition now that it should check parent is root and allowTopLevel is enabled. i updated the definition of parentIsRoot above to be safe: https://github.com/scarf-sh/scarf-js/pull/61/files#diff-7f51467a2b2b5a7527294c07a9a35c8f3195ad9e4ced1ab2c5d1450a75840713R77

}

const rootPackage = dependencyInfo.rootPackage

if (!userHasOptedIn(rootPackage) && isYarn()) {
Expand Down Expand Up @@ -512,8 +546,10 @@ module.exports = {
tmpFileName,
dirName,
processDependencyTreeOutput,
processGitRevParseOutput,
npmExecPath,
getDependencyInfo,
getGitShaFromRootPath,
reportPostInstall,
hashWithDefault,
findScarfInFullDependencyTree
Expand Down
10 changes: 10 additions & 0 deletions test/report.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,16 @@ describe('Reporting tests', () => {
return report.processDependencyTreeOutput(resolve, reject)(null, JSON.stringify(parsedLsOutput), null)
})).rejects.toEqual(new Error('The package depending on Scarf is the root package being installed, but Scarf is not configured to run in this case. To enable it, set `scarfSettings.allowTopLevel = true` in your package.json'))
})

test('Can parse example git rev-parse HEAD output', async () => {
await expect(new Promise((resolve, reject) => {
return report.processGitRevParseOutput(resolve, reject)(null, '9ace16b9e3833ad4e761a49f17fe607723d5bd5e\n', null)
})).resolves.toEqual('9ace16b9e3833ad4e761a49f17fe607723d5bd5e')
})

test('getGitShaFromRootPath resolves in test run of cloned repository', async () => {
await expect(report.getGitShaFromRootPath()).resolves.toBeTruthy()
})
})

function dependencyTreeScarfEnabled () {
Expand Down
Loading