CachingProjectLoader should respect clone parameters

[timothysparg]

Resolves #729

[timothysparg]

@ddgenome Is this in line with what you were thinking?

[cdupuis]

I think what’s missing here is the entire set of loading parameters to be considered when using the cache key. Right now only a subset of parameters gets used when caching.

[timothysparg]

ok, that makes sense. Let me rework this.

[cdupuis]

@timothysparg any chance to get his rebased?

[timothysparg]

@cdupuis no problem, will do

[timothysparg]

@cdupuis should be up to date with master again

[ddgenome]

@timothysparg the manual edit of the package.json made it out of sync with the package-lock.json. You can verify by running npm ci.

$ npm ci
npm WARN prepare removing existing node_modules/ before installation
npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
npm ERR!
npm ERR!
npm ERR! Invalid: lock file's @atomist/[email protected] does not satisfy @atomist/[email protected]
npm ERR!

npm ERR! A complete log of this run can be found in:
npm ERR!     /XXX/YY/.npm/_logs/2020-02-11T22_21_13_358Z-debug.log

[ddgenome]

Thank you for your contribution!

The package-lock.json issue needs to be addressed. We may want to consider optimizing undefined/null clone option properties.

https://github.com/atomist/automation-client/blob/master/lib/spi/clone/DirectoryManager.ts

[ddgenome]

Since these are optional, we may have erroneous cache misses because this approach does not recognize, for example, that alwaysDeep having a value undefined or false is equivalent. This should only be a performance issue, so it could perhaps be addressed later.

[ddgenome]

Thanks for the fixes, one issue remains.

[ddgenome]

This approach would work for the boolean properties, but not for depth. Perhaps something more direct would be better:

params.cloneOptions?.keep || false,
params.cloneOptions?.alwaysDeep || false,
params.cloneOptions?.noSingleBranch || false,
params.cloneOptions?.depth || 1,
params.cloneOptions?.detachHead || false,

With defaults derived from the code

• https://github.com/atomist/automation-client/blob/master/lib/project/git/GitCommandGitProject.ts#L355
• https://github.com/atomist/automation-client/blob/5b43b968067206df081791d3cca6456eeebeeadf/lib/spi/clone/DirectoryManager.ts#L40-L43

[timothysparg]

works for me, will update and revert.

Sanity check - It seems like we can assume in this context that cloneOptions will always be defined?

[ddgenome]

If it isn't, the ?. will handle it.

[ddgenome]

Thanks!