Skip to content

chore: update the DryRunService to improve error messages #4001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

chore: update the DryRunService to improve error messages #4001

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/cli/lib/services/dryrun.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,15 +157,30 @@ export class DryRunService {
}

// Priority for syncs and actions
let foundInProviderConfigKey: string | undefined;
for (const script of [...integration.syncs, ...integration.actions]) {
if (script.name !== syncName) {
continue;
}
if (scriptInfo) {
console.log(chalk.red(`Multiple integrations contain a script named "${syncName}". Please use "--integration-id"`));
// TODO: Confirm: If exists another script with the same name in the same integration, it's a variant issue
if (foundInProviderConfigKey === providerConfigKey) {
console.log(
chalk.red(
`Multiple variants of script "${syncName}" found in integration "${providerConfigKey}". Please specify which variant to use with "--variant"`
)
);
} else {
console.log(
chalk.red(
`Script "${syncName}" exists in multiple integrations (${foundInProviderConfigKey}, ${providerConfigKey}). Please specify which integration to use with "--integration-id"`
)
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure I follow the logic. I don't think you can detect if variants exist unless querying the API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, wasn't too sure about this one.
if we cant distinguish between the two should it just be an update to the existing error message to include variant?

Multiple integrations contain a script named "${syncName}". Please use "--integration-id or --variant to specify"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I don't really understand what the problem is with variants. Based on the code above it looks like if a variant is not specified the CLI default to the base one so I am not sure I understand the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me dig into this further get what the root cause of this request was

return;
}
scriptInfo = script;
foundInProviderConfigKey = providerConfigKey;
providerConfigKey = integration.providerConfigKey;
Comment on lines 182 to 184
Copy link

Choose a reason for hiding this comment

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

[DataTypeCheck]

There's a potential issue with the assignment of providerConfigKey. You're assigning providerConfigKey to foundInProviderConfigKey and then reassigning providerConfigKey immediately after. This could lead to both variables having the same value by the next iteration, which might cause incorrect comparisons in the if (foundInProviderConfigKey === providerConfigKey) condition.

Consider storing the original value before reassigning:

scriptInfo = script;
foundInProviderConfigKey = providerConfigKey;
providerConfigKey = integration.providerConfigKey;

}

Expand Down
Loading