Skip to content
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.

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

change: add a more clear error for duplicate project creation #72

Merged
merged 5 commits into from
Dec 13, 2023

Conversation

MikePresman
Copy link
Contributor

Whenever a project is attempted to made again the database throws a uniqueness error. This usually comes not so clearly out clientside and as such this change implements a cleaner error message for handling recreating projects that may occur in instances such as job retries with multiple steps.

@MikePresman MikePresman force-pushed the change/idempotent-project-create branch from bcecc2d to 9fce6f0 Compare December 12, 2023 15:55
@MikePresman MikePresman force-pushed the change/idempotent-project-create branch from 9fce6f0 to f61ed37 Compare December 12, 2023 15:56
@MikePresman MikePresman changed the title change: add more clear for duplicate project change: add a more clear error for duplicate project creation Dec 12, 2023
@angelini
Copy link
Contributor

Can you also update the JS grpc-client to throw a nice error like ProjectAlreadyExists

Copy link
Collaborator

@scott-rc scott-rc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines 135 to 139
if (error instanceof RpcError && error.code == "ALREADY_EXISTS") {
throw new ProjectAlreadyExistsError(`project id ${project} already exists`);
} else {
throw error;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: no need for an else block here

Suggested change
if (error instanceof RpcError && error.code == "ALREADY_EXISTS") {
throw new ProjectAlreadyExistsError(`project id ${project} already exists`);
} else {
throw error;
}
if (error instanceof RpcError && error.code == "ALREADY_EXISTS") {
throw new ProjectAlreadyExistsError(`project id ${project} already exists`);
}
throw error;

Comment on lines 2 to 4
constructor(msg: string) {
super(msg);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: no need to re-implement the same constructor

Suggested change
constructor(msg: string) {
super(msg);
}

@MikePresman MikePresman merged commit a574f7d into main Dec 13, 2023
4 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.

3 participants