get git sha on install #61
Conversation
| if (allowTopLevel(dependencyInfo.rootPackage)) { | ||
| const gitSha = await getGitSha() | ||
| logIfVerbose(`Injecting sha to parent: ${gitSha}`) | ||
| dependencyInfo.parent.gitSha = gitSha |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -226,6 +242,18 @@ async function getDependencyInfo (packageJSONOverride) { | |||
| }) | |||
| } | |||
|
|
|||
| async function getGitSha () { | |||
| const promise = new Promise((resolve, reject) => { | |||
| exec(`cd ${rootPath} && git rev-parse HEAD`, { timeout: execTimeout, maxBuffer: 1024 * 1024 * 1024 }, processGitRevParseOutput(resolve, reject)) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i at least updated the function name that this only gets the root path git sha
* fix up parentIsRoot to be safe against optional values
justinwoo
force-pushed
the
get-git-sha-on-install
branch
from
879df0c
to
aa5fe40
Compare
We should try to get the git sha of the root and add it to the reporting data if possible