Skip to content

Conversation

@tburdge
Copy link

@tburdge tburdge commented Dec 11, 2024

No description provided.

@tburdge tburdge requested a review from phalestrivir December 11, 2024 22:40
"uuid": "^9.0.0",
"yesno": "^0.4.0"
},
"dependencies": {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be devDependencies?

)
.addOption(
new Option(
'-E, --frodo-export-dir <directory>',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd lean towards lowercase flags when possible.

)
.addOption(
new Option(
'--propmt-prune',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling

)
.addOption(
new Option(
'-S --effect-secrets',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling

)
.addOption(
new Option(
'--target <host url>',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? The promotion staging should all be for the single environment for now.

);
if (
(await getTokens(false, true, deploymentTypes)) &&
options.masterDir &&
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reorder to validate option prior to making the request for tokens

break;
}
case 'resourcetype': {
const keysToRemove = [
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we just remove the metadata instead of individual keys?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make a method that does something similar to that but these export with the metadata even when the flag is there to remove the metadata because I think they are part of the object in a different way

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when the exports are done, some objects do have the metadata as part of them. The saveToFile and saveJsonToFile functions that save the export are what eventually remove it (if it exists) or add them (if it doesn't exist) to the export when it is written to a file. You can simply do delete obj.meta to delete the metadata if it exists. However, I realized these keys might be attributes of the object instead and not part of the meta object, in which case we probably do need to have it remove the individual keys if we want to ignore those.

export default function setup() {
const program = new FrodoCommand('frodo idm delete');

interface ServiceDeleteOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left over dead code from copying the service command.

}

program
.description('Delete AM services.')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update descriptions to be descriptive of idm and not services

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDM delete command is not necessary for the promotion, this should be in a separate PR

command
);

// const globalConfig = options.global ?? false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDM delete command is not necessary for the promotion, this should be in a separate PR

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full delete is not necessary but I did have to pull inn the command from the library

const agent = getJsonObjectTwoDown(importFilePath);
const agentType = agent._type._id;
verboseMessage(`Agent id: ${agent._id} and type: ${agentType}`);
switch (agentType) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switch statement isn't necessary for agents, I would just stick with the default case since the default case will work for all agent types include the web, java, and IG agents.

break;
}
// Taken care of by Idm
case 'theme': {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine the cases so that we only break in one place (like I mentioned previously)

verboseMessage(
`Deleting agent '${agent._id}' of type ${agentType} in realm "${state.getRealm()}"...`
);
switch (agentType) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch statement not necessary here, default case should suffice for all agent types

const type = getTypeFromPath(path);
let promotable: boolean;
switch (type) {
case 'cot': {
Copy link
Collaborator

@phalestrivir phalestrivir Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three cases can be combined into a single case. In fact, since the switch statement isn't all that big, you could probably just do an if statement if you want to.

@hfranklin hfranklin merged commit 462d77d into main Mar 5, 2025
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants