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

Handle file deletion in incremental load PullEventHandler and throw errors (if any) instead of returning Success Status #320

Merged
merged 6 commits into from
Jan 26, 2024

Conversation

isc-vgao
Copy link
Contributor

This fixes #299 , #300
Testing Steps:

  1. Delete a class in a repo.
  2. Run
w ##class(SourceControl.Git.API).Pull()
  1. Verify that no error messages (Error # ...) are printed in terminal. Instead, "<name of file deleted in step 1> was not imported into the database and will not be compiled." should be printed in terminal.

@isc-vgao isc-vgao self-assigned this Jan 24, 2024
Copy link
Collaborator

@isc-tleavitt isc-tleavitt left a comment

Choose a reason for hiding this comment

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

Great simple solution - thank you! Just one minor change needed.

cls/SourceControl/Git/PullEventHandler/IncrementalLoad.cls Outdated Show resolved Hide resolved
Copy link
Collaborator

@isc-tleavitt isc-tleavitt left a comment

Choose a reason for hiding this comment

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

One final note on error handling: as implemented we ignore whether the item was deleted or not, and then always say it was deleted. We should instead attempt to delete the item, and warn (without throwing an exception) if the delete fails (most likely because the item had already been deleted / didn't exist). So DeleteFile should return the status (possibly an error) from the Delete method used, but IncrementalLoad shouldn't $$$ThrowOnError - should just print a warning instead of "was deleted" if the status was an error.

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.

Incremental load PullEventHandler needs to handle deletes
2 participants