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 more buildplanner errors #3547

Closed
wants to merge 5 commits into from
Closed

Handle more buildplanner errors #3547

wants to merge 5 commits into from

Conversation

MDrakos
Copy link
Member

@MDrakos MDrakos commented Oct 17, 2024

TaskDX-2821 Handle Buildplanner errors with user facing messages

@MDrakos MDrakos changed the base branch from master to version/0-47-0-RC1 October 17, 2024 20:52
}
// ValidationError represents a validation error that occurred during planning.
// Contains message and subErrors which is handled separately
type ValidationError struct{}
Copy link
Member Author

Choose a reason for hiding this comment

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

We now have multiple errors with SubErrors so I moved that to the common Error struct

@MDrakos
Copy link
Member Author

MDrakos commented Oct 18, 2024

I found that a lot of the errors we are currently handling wouldn't be very enriched by the extra fields. I also took the time in this PR to handle errors that we weren't before.

@MDrakos MDrakos requested a review from Naatan October 18, 2024 20:02
Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

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

We should be using the rationalizer for this. These are already returning typed errors, which are at least to some level already rationalized. Adding localized errors underneath them feels inappropriate. We should only be marking input errors outside of rationalizers if there is a clear reason why rationalizing doesn't work there.

@MDrakos
Copy link
Member Author

MDrakos commented Oct 24, 2024

Closing this PR and filed another story that I think will better address our buildplanner error handling.

@MDrakos MDrakos closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants