-
Notifications
You must be signed in to change notification settings - Fork 626
[rush-lib] Add support for custom CredentialCache
paths
#5240
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
base: main
Are you sure you want to change the base?
Conversation
The `CredentialCache` does not rely on the Rush user configuration. It can be used outside of `@rush/rush-sdk` by directly importing it from `@microsoft/rush-lib`. To support such use cases, we should allow the consumer to specify a file path for their cache, rather than the fixed `~/.rush-user/credentials.json` path. Non-breaking change because the default path remains the same, and we add support for additional options in `ICredentialCacheOptions`. | `ICredentialCacheOptions.cacheDirectory` | `.cacheName` | Resulting Path | |-|-|-| | `undefined` | `undefined` | `~/.rush-user/credentials.json` (default) | | `undefined` | `custom-name` | `~/.rush-user/custom-name.json` | | `/custom-dir` | `undefined` | `/custom-dir/credentials.json` | | `/custom-dir` | `custom-name` | `/custom-dir/custom-name.json` |
{ | ||
testCaseName: 'default cache directory with default cache name', | ||
partialOptions: {}, | ||
expectedCacheFilePath: FAKE_DEFAULT_CREDENTIALS_CACHE_FILE | ||
}, | ||
{ | ||
testCaseName: 'default cache directory with custom cache name', | ||
partialOptions: { cacheName: 'my-cache-name' }, | ||
expectedCacheFilePath: `${FAKE_RUSH_USER_FOLDER}/my-cache-name.json` | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could move the jest.spyOn(RushUserConfiguration, 'getRushUserFolderPath')
mock to only return a value in these two test cases, and throw an error in the others (because it should not be called).
common/reviews/api/rush-lib.api.md
Outdated
// (undocumented) | ||
cacheDirectory?: string; | ||
// (undocumented) | ||
cacheName?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a folder + file base name, why not just a path to the credential cache file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep agreed! Path to credential cache file is a more friendly API. Will change it now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only use case is for a consumer to create another cache in the same folder as ~/.rush-user/credentials.json
, which can be achieved by only specifying the cacheName
. I don't see this being helpful because consumers that want a custom cache will likely want to have full control over the path.
let cacheDirectory: string; | ||
let cacheName: string; | ||
if (options.cacheFilePath) { | ||
cacheDirectory = path.dirname(options.cacheFilePath); | ||
cacheName = path.basename(options.cacheFilePath, '.json'); | ||
} else { | ||
cacheDirectory = RushUserConfiguration.getRushUserFolderPath(); | ||
cacheName = DEFAULT_CACHE_NAME; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would enforce const
correctness and avoid repeating the options.cacheFilePath ?
condition, but is slightly hard to read.
const [cacheDirectory, cacheName] = options.cacheFilePath
? [path.dirname(options.cacheFilePath), path.basename(options.cacheFilePath, '.json')]
: [RushUserConfiguration.getRushUserFolderPath(), DEFAULT_CACHE_NAME];
Wdyt @iclanton?
Reverts unintended change in lockfile path.
Summary
The
CredentialCache
does not rely on the Rush user configuration; it can be used outside@rush/rush-sdk
by importing from@microsoft/rush-lib
instead. To support such use cases, we should allow the consumer to pass a custom file path for their cache, rather than using the fixed~/.rush-user/credentials.json
path. This allowsCredentialCache
to be used in non-Rush codebases, or when we prefer a different location.Details
Adds the following options to
ICredentialCacheOptions
:.cacheDirectory
.cacheName
undefined
undefined
~/.rush-user/credentials.json
(default)undefined
custom-name
~/.rush-user/custom-name.json
/custom-dir
undefined
/custom-dir/credentials.json
/custom-dir
custom-name
/custom-dir/custom-name.json
When neither
cacheDirectory
norcacheName
are supplied, the path defaults to the same location we currently use (~/.rush-user/credentials.json
). So this is non-breaking.How it was tested
Added new tests to verify that default/custom paths are respected in each of the above
ICredentialCacheOptions
combinations.Impacted documentation
Only the extracted API is affected: https://api.rushstack.io/pages/rush-lib.credentialcache/.