Feature: Add name flag to a config-manager pull commands#118
Feature: Add name flag to a config-manager pull commands#118brycentrivir wants to merge 1 commit into
Conversation
741ad04 to
1572f13
Compare
brycentrivir
changed the title
| let outcome: boolean; | ||
| if (options.name) { | ||
| verboseMessage(`Exporting ${options.name}`); | ||
| outcome = await configManagerExportMappings(options.name); | ||
| } else { | ||
| verboseMessage('Exporting connector mappings'); | ||
| outcome = await configManagerExportMappings(); | ||
| } |
There was a problem hiding this comment.
Since you are calling the same function, you don't need an if statement here. You can simplify this to:
verboseMessage(options.name ? `Exporting ${options.name}` : 'Exporting connector mappings');
const outcome = await configManagerExportMappings(options.name);| let outcome: boolean; | ||
| if (options.name) { | ||
| verboseMessage(`Exporting ${options.name}`); | ||
| outcome = await configManagerExportCsp(options.file, options.name); | ||
| } else { | ||
| verboseMessage('Exporting content security policy'); | ||
| outcome = await configManagerExportCsp(options.file); | ||
| } |
There was a problem hiding this comment.
Since you are calling the same function, you don't need an if statement here. You can simplify this to:
verboseMessage(options.name ? `Exporting ${options.name}` : 'Exporting content security policy');
const outcome = await configManagerExportCsp(options.file, options.name);| program | ||
| .description('Export connector mappings.') | ||
| .addOption( | ||
| new Option('-n, --name <name>', 'Export by name of connector mapping.') |
There was a problem hiding this comment.
Suggestion: We can make the wording more concise with Export by name. or Export connector mapping by name. Technically "connector mapping" is not necessary as that is implied by the command, although it's fine if we keep it in there.
| 'The CSP_OVERRIDES json file. ex: "/home/trivir/Documents/csp-overrides.json", or "csp-overrides.json"' | ||
| ) | ||
| ) | ||
| .addOption(new Option('-n, --name <name>', 'Export by name of csp.')) |
There was a problem hiding this comment.
Suggestion: Similar to https://github.com/trivir/frodo-cli/pull/118/changes#r3580098075
| '-r, --realm <realm>', | ||
| 'Specifies the realm to export from. Only the entity object from this realm will be exported.' | ||
| ) | ||
| new Option('-n, --name <name>', 'Export by name of org-privilege') |
There was a problem hiding this comment.
Suggestion: Similar to https://github.com/trivir/frodo-cli/pull/118/changes#r3580098075
| * @param theme theme | ||
| * @param themePath theme path |
|
|
||
| export async function configManagerExportThemes(): Promise<boolean> { | ||
| /** | ||
| * Exports all themes or via name / id |
There was a problem hiding this comment.
We are exporting by name, not by id, so remove the / id part
|
|
||
| /** | ||
| * Export all variables to seperate files | ||
| * Export all variables to seperate files. If named param include, export only the one named variable. |
There was a problem hiding this comment.
I would fix the grammar on this. It should be something like If name specified, export only the specified variable.
| const privilegeName = 'alphaOrgPrivileges'; | ||
| const CMD = `frodo config-manager pull org-privileges -D ${dirName} -n ${privilegeName}`; | ||
| await testExport(CMD, env, undefined, undefined, dirName, false); | ||
| }); |
There was a problem hiding this comment.
For org-privileges, we should have a test for forgeops as well without the -n flag to ensure it will export the root realm as well as an alpha/bravo realm org privileges it has.
I would use the test configuration from here since it has configuration for the different org privileges and realms. Just make sure your forgeops tenant has an alpha and bravo realm in it before you import
| "content": { | ||
| "mimeType": "application/json", | ||
| "size": 331, | ||
| "text": "{\"_id\":\"esv-goto-urls\",\"description\":\"List of valid goto urls for post login\",\"expressionType\":\"array\",\"lastChangeDate\":\"2025-02-11T20:18:49.268301Z\",\"lastChangedBy\":\"Frodo-SA-1739303176438\",\"loaded\":true,\"valueBase64\":\"WyJodHRwczovL3dlYmRldjEuaGVhbHRocGFydG5lcnMuY29tLyoiLCJodHRwczovL3dlYmRldjEuaGVhbHRocGFydG5lcnMuY29tLyo/KiJd\"}" |
There was a problem hiding this comment.
I would re-record this test using a different test ESV that you create, this ESV contains client related information
|
Sorry, one more thing to check for that I noticed in the realms PR, make sure any command that does have a name option should have a name option. If the corresponding command in fr-config-manager does not have a name flag and ours does, we should delete that flag from ours. |
2 participants
A couple of commands were missing the -n, --name flag in frodo's version of config-manager, so these changes add the flag to those couple of commands
List of commands:
connector-mappings
csp
org-privileges
secrets
themes
variables