feat: add -a, --active-only to export secrets#119
Conversation
There was a problem hiding this comment.
You forgot to run npm run lint:fix (that's how I caught the console.log that you left in)
Also, I would update the config-manager-export-all test's mock and snapshot. This is because, if the test was passing under normal circumstances, it would be failing now as a result of your change since you changed how the ESV secrets get exported to now include versions. The tests are failing due to other changes too, so fixing the test would fix both those issues as well as the issue caused by updating the ESV secrets. You should be able to just re-record the mock for it and update the snapshot and then it will pass.
| type FrConfigSecret = SecretSkeleton & { | ||
| valueBase64: string; | ||
| }; | ||
|
|
||
| async function getFrConfigSecrets(): Promise<FrConfigSecret[]> { | ||
| const originalSecrets = await readSecrets(); | ||
| return originalSecrets.map((secret) => ({ | ||
| ...secret, | ||
| valueBase64: `\${${secret._id.toUpperCase().replace(/-/g, '_')}}`, | ||
| })); | ||
| interface CleanSecretVersionInterface { | ||
| version: string; | ||
| status: VersionOfSecretStatus; | ||
| valueBase64: string; | ||
| } | ||
|
|
||
| interface CleanSecretInterface { | ||
| _id: string; | ||
| description: string; | ||
| encoding: SecretEncodingType; | ||
| useInPlaceholders: boolean; | ||
| valueBase64?: string; | ||
| versions?: CleanSecretVersionInterface[]; | ||
| } |
There was a problem hiding this comment.
I would delete these interfaces/types as they are not necessary (see my other comment)
7c94222 to
5c7cb8c
Compare
phalestrivir
left a comment
phalestrivir
left a comment
There was a problem hiding this comment.
Just a few minor changes needed
| .addOption( | ||
| new Option( | ||
| '-a, --active-only', | ||
| 'Exports only active secrets, otherwise exports all secrets and their versions.' |
There was a problem hiding this comment.
I think we should only have it say Export only active secret versions. It's implied that if it's not provided all secret versions will be exported.
| }); | ||
| test('"frodo config-manager pull secrets -aD secretTestDir2": should export all the active only secrets in fr-config-manager style"', async () => { | ||
| const dirName = 'secretTestDir2'; | ||
| const CMD = `frodo config-manager pull secrets -D ${dirName}`; |
There was a problem hiding this comment.
This test is wrong, it should have -aD here.
| test('"frodo config-manager pull all -F test/e2e/fr-config-manager-pull-config -D allDir1": should export all config in fr-config-manager style"', async () => { | ||
| const dirName = 'allDir1'; | ||
| const CMD = `frodo config-manager pull all -F test/e2e/fr-config-manager-pull-config -D ${dirName}`; | ||
| const fileName= 'test/e2e/fr-config-manager-pull-config'; |
There was a problem hiding this comment.
Technically this is a directory name, not a file name, so I'd change the name of this variable to reflect that. It's the directory (or folder) which contains all the config-manager configurations for pulling.
Note in the future I think we are going to have to change how the export all works, since we should be able to specify where each of the individual configuration files are when doing a pull rather than a directory, especially since that's how config-manager does it, so this is only going to be a temporary thing.
| type FrConfigSecret = SecretSkeleton & { | ||
| valueBase64: string; | ||
| }; | ||
|
|
There was a problem hiding this comment.
We don't need this type either, so I would just remove it. If you delete all the places we are using it you shouldn't run into any errors
2 participants
Adds the -a flag to config-manager pull secrets.
Changes within file: