bugfix: realm flag removed for config-manager#116
Conversation
brycentrivir
changed the title
phalestrivir
left a comment
phalestrivir
left a comment
There was a problem hiding this comment.
After the changes, make sure to update the tests accordingly. Main thing to remember is that for those commands that due use the realm argument we need to have a separate test where we specify the realm that way, which it looks like you already did, but I won't look over the tests until these changes are done since the tests I know are going to change for a lot of these. For example, all-static has the realm test that you added which is good, but we never did implement the realm argument for it (which I left a comment about), so once you implement that you will need to re-record that test since it currently doesn't test anything.
| [], | ||
| deploymentTypes | ||
| ); | ||
| // TO DO: adding a realm option to export all-static for specific realm |
There was a problem hiding this comment.
Still needs implementation for realm argument
| [], | ||
| deploymentTypes | ||
| ); | ||
| // TO DO: Adding a realm option to export all config for a specific realm |
There was a problem hiding this comment.
Still needs implementation of realm argument
| .addOption( | ||
| new Option( | ||
| '-n, --policy-name <policy-set-name>', | ||
| 'Get only a specific policy set with the name.' | ||
| ) | ||
| ) |
| options.file | ||
| ); | ||
|
|
||
| // check other realms for the script but only if there is no config file specified | ||
| if (!outcome && !options.file) { | ||
| const checkedRealms: string[] = [state.getRealm()]; | ||
| for (const realm of await readRealms()) { | ||
| if (outcome) { | ||
| break; | ||
| } | ||
| if (!checkedRealms.includes(realm.name)) { | ||
| printMessage( | ||
| `Exporting the policy set "${options.policyName}" from the ${checkedRealms[checkedRealms.length - 1]} realm failed.` | ||
| ); | ||
| state.setRealm(realm.name); | ||
| checkedRealms.push(state.getRealm()); | ||
| printMessage( | ||
| `Looking for the policy set "${options.policyName}" in the ${state.getRealm()} realm now.` | ||
| ); | ||
| outcome = await configManagerExportAuthzPolicySet( | ||
| { | ||
| policySetName: options.policyName, | ||
| }, | ||
| null | ||
| ); | ||
| } | ||
| } | ||
| if (!outcome) { | ||
| printMessage( | ||
| `Did not find the policy set "${options.policyName}" anywhere.` | ||
| ); | ||
| } | ||
| } | ||
| } |
| else if (options.file) { | ||
| printMessage( | ||
| `Exporting all the policy sets in the provided config file.` | ||
| ); | ||
| outcome = await configManagerExportAuthzPolicySets(options.file); | ||
| } | ||
|
|
||
| // -r/--realm | ||
| // realm argument | ||
| else if (realm !== constants.DEFAULT_REALM_KEY) { | ||
| printMessage( | ||
| `Exporting all the policy sets in the ${state.getRealm()} realm.` | ||
| ); | ||
| outcome = await configManagerExportAuthzPolicySetsRealm(); | ||
| } | ||
|
|
||
| // export all policy sets from all realms, the default when no options are provided | ||
| else { | ||
| printMessage('Exporting all the policy sets in the host tenant.'); | ||
| outcome = await configManagerExportAuthzPoliciesAll(); | ||
| } |
There was a problem hiding this comment.
I do think we should refactor this to all use the same function as we do in other places to help keep it simple. One function that takes in a file (if it provided), and does the exports accordingly.
We do NOT need the realm case, there is no realm flag in the original fr-config-pull command, so remove all realm specific code. The realms are specified in the file.
| outcome = | ||
| (await configManagerExportOrgPrivileges()) && | ||
| (await configManagerExportOrgPrivilegesRealm(realm)); | ||
| (await configManagerExportOrgPrivilegesRealm(realm)); |
There was a problem hiding this comment.
We need to remove the option for a realm argument here, the original command does NOT have a -r flag for it.
| ) | ||
| .addOption( | ||
| new Option( | ||
| '-n, --script-name <script name>', |
There was a problem hiding this comment.
In your name PR, I would modify this to just be --name to match fr-config-manager
| .addOption( | ||
| new Option( | ||
| '-p, --prefix <prefix>', | ||
| 'Export all scripts that start with a certain prefix. Ignored with -n' | ||
| ) | ||
| ) | ||
| .addOption( | ||
| new Option( | ||
| '--just-content', | ||
| 'Export only the script .js files, no config files' | ||
| ) | ||
| ) | ||
| .addOption( | ||
| new Option( | ||
| '--just-config', | ||
| 'Export only the config .json files, no scripts. Ignored with --just-content' | ||
| ) | ||
| ) | ||
| .addOption( | ||
| new Option( | ||
| '--script-type <script type>', | ||
| 'Export all scripts of a certain type. Ignored with -n.' | ||
| ) | ||
| ) | ||
| .addOption( | ||
| new Option( | ||
| // the two options are groovy and all because the command "fr-config-pull scripts" in fr-config manager with no specified arguements returns only js files | ||
| '--language <programming language>', | ||
| 'Export all scripts written a certain programming language. ALL, GROOVY, or JAVASCRIPT. defaults to JAVASCRIPT. Ignored with -n' | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Also, none of these should be here, I don't know who implemented this command, but these don't exist in regular fr-config-manager, so we don't need them
| // -n/--script-name | ||
| if (options.scriptName) { | ||
| // try and find script in current realm | ||
| printMessage( | ||
| `Exporting script ${options.scriptName} from the ${state.getRealm()} realm.` | ||
| ); | ||
| const originalRealm: string = state.getRealm(); | ||
| outcome = await configManagerExportScript( | ||
| { | ||
| scriptName: options.scriptName, | ||
| }, | ||
| options.justContent, | ||
| options.justConfig | ||
| ); | ||
|
|
||
| // check other realms for the script | ||
| if (!outcome) { | ||
| for (const realm of await readRealms()) { | ||
| if (outcome) { | ||
| break; | ||
| } | ||
| if (realm.name !== originalRealm) { | ||
| printMessage( | ||
| `Exporting script ${options.scriptName} from the ${state.getRealm()} realm failed.` | ||
| ); | ||
| state.setRealm(realm.name); | ||
| printMessage( | ||
| `Looking for the ${options.scriptName} script in the ${state.getRealm()} realm now.` | ||
| ); | ||
| outcome = await configManagerExportScript( | ||
| { | ||
| scriptName: options.scriptName, | ||
| }, | ||
| options.justContent, | ||
| options.justConfig | ||
| ); | ||
| } | ||
| } | ||
| if (!outcome) { | ||
| printMessage( | ||
| `Did not find the script "${options.scriptName}" anywhere.` | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // -r/--realm | ||
| // realm argument | ||
| else if (realm !== constants.DEFAULT_REALM_KEY) { | ||
| printMessage( | ||
| `Exporting scripts from the ${state.getRealm()} realm${hasOptions ? ' with custom options.' : '.'}` | ||
| ); | ||
| outcome = await configManagerExportScriptsRealms( | ||
| options.prefix, | ||
| options.justContent, | ||
| options.justConfig, | ||
| options.scriptType, | ||
| options.language | ||
| ); | ||
| } | ||
|
|
||
| // export all, the default | ||
| else { | ||
| printMessage( | ||
| `Exporting scripts from entire tenant${hasOptions ? ' with custom options.' : '.'}` | ||
| ); | ||
| outcome = await configManagerExportScriptsAll( | ||
| options.prefix, | ||
| options.justContent, | ||
| options.justConfig, | ||
| options.scriptType, | ||
| options.language | ||
| ); | ||
| } | ||
|
|
||
| if (!outcome) { | ||
| process.exitCode = 1; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I would refactor this to all use the same function that takes in the script name and the realm.
| @@ -18,11 +18,18 @@ export async function configManagerExportOrgPrivilegesRealm( | |||
| realm: string | |||
| ): Promise<boolean> { | |||
There was a problem hiding this comment.
Again, no realm flag should exist for org privileges (I think this was fixed though in your names PR, that code should be moved over here)
2 participants
Removal of
-r, --realmflag throughout config-manager pull commands as there was a redundancy created as Frodo already has the realm argument built in.