Skip to content

Conversation

@skootrivir
Copy link

No description provided.

@skootrivir skootrivir changed the title Added deleteServiceNext Added deleteServiceNextDescendentOnly function Apr 30, 2025
@phalestrivir phalestrivir self-requested a review April 30, 2025 23:13
@phalestrivir phalestrivir force-pushed the trivir branch 3 times, most recently from 94375fc to ebf6df8 Compare May 12, 2025 19:05
Copy link
Collaborator

@phalestrivir phalestrivir left a comment

Choose a reason for hiding this comment

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

Let me know if you have any questions about the changes I requested. You will need to update the CLI PR as well with the renaming for the deleteServiceNextDescendants function.

@phalestrivir phalestrivir changed the base branch from trivir to main June 5, 2025 16:55
Copy link
Collaborator

@phalestrivir phalestrivir left a comment

Choose a reason for hiding this comment

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

I just reviewed the PR again, and I'm thinking the best thing to do here is instead of providing a flag to delete just the descendents, we just create a separate function that does it and export that as part of the lib so you can use it in the CLI. The reasoning is that we shouldn't do it as part of the deleteFullService function since that function in particular is meant to delete the entire service including next descendents. Plus, doing this change means you only need to update ServiceOps and none of the other files.

To make the change, you can get rid of all your current changes in this PR, and just add the following function:

/**
 * Deletes the specified service's descendents
 * @param {string} serviceId The service whose descendents you want to delete
 * @param {boolean} globalConfig true if the global service is the target of the operation, false otherwise. Default: false.
 */
export async function deleteServiceNextDescendents({
  serviceId,
  globalConfig = false,
  state,
}: {
  serviceId: string;
  globalConfig: boolean;
  state: State;
}) {
  try {
    debugMessage({
      message: `ServiceOps.deleteServiceNextDescendents: start, globalConfig=${globalConfig}`,
      state,
    });
    const serviceNextDescendentData = await getServiceDescendents({
      serviceId,
      globalConfig,
      state,
    });

    const descendents = await Promise.all(
      serviceNextDescendentData.map((nextDescendent) =>
        deleteServiceNextDescendent({
          serviceId,
          serviceType: nextDescendent._type._id,
          serviceNextDescendentId: nextDescendent._id,
          globalConfig,
          state,
        })
      )
    );

    debugMessage({
      message: `ServiceOps.deleteServiceNextDescendents: end`,
      state,
    });
    return descendents;
  } catch (error) {
    throw new FrodoError(
      `Error deleting ${
        globalConfig ? 'global' : 'realm'
      } service descendents ${serviceId}`,
      error
    );
  }
}

You also want to update deleteFullService to use this function to reduce code redundancy:

/**
 * Deletes the specified service
 * @param {string} serviceId The service to delete
 * @param {boolean} globalConfig true if the global service is the target of the operation, false otherwise. Default: false.
 */
export async function deleteFullService({
  serviceId,
  globalConfig = false,
  state,
}: {
  serviceId: string;
  globalConfig: boolean;
  state: State;
}) {
  try {
    debugMessage({
      message: `ServiceOps.deleteFullService: start, globalConfig=${globalConfig}`,
      state,
    });

    await deleteServiceNextDescendents({
      serviceId,
      globalConfig,
      state,
    });

    debugMessage({ message: `ServiceOps.deleteFullService: end`, state });
    return deleteService({ serviceId, globalConfig, state });
  } catch (error) {
    throw new FrodoError(
      `Error deleting ${
        globalConfig ? 'global' : 'realm'
      } full service config ${serviceId}`,
      error
    );
  }
}

Then you will need to add the deleteServiceNextDescendents function as part of the exported functions in this file so you can use it in the CLI, then update your CLI PR with the changes.

@hfranklin
Copy link

@phalestrivir, do you think this is something we could reasonably write unit tests for?

@phalestrivir
Copy link
Collaborator

@phalestrivir, do you think this is something we could reasonably write unit tests for?

@hfranklin Yes, we could, it would just involve making a recording where we delete all the descendents for one of the services in the frodo dev tenant, which shouldn't cause any issues if we create one that isn't being used anywhere that happens to have descendent configuration. @skootrivir Would you be able to write a unit test or two for this as part of the changes I suggested?

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.

3 participants