-
Notifications
You must be signed in to change notification settings - Fork 13
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
Recognize invalid input as a buildplanner error response. #3622
base: version/0-48-0-RC1
Are you sure you want to change the base?
Conversation
The scan failure will be resolved if we decide to backport some CVE fixes from v48 into this v47 branch. |
The Target & Verify failure is due to something I don't understand:
There seems to be something wrong with the v47 RC2 branch maybe? |
@mitchell-as it was failing because the RC2 branch didn't exist, and it failed to create it for whatever reason. But I don't think we'll do an RC2, I've repurposed this for v48. Please update your PR accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this addresses the expected behaviour in the bug report, unless this is implying a mechanic I'm remembering?
@Naatan without this fix, you get the "Command failed due to unexpected error", which is thoroughly unhelpful and reported to rollbar. With this fix, you'll get an input error that tells you your project has unexpected characters in it. |
That's a good change, I'm all for it. But the bug is raising a different issue altogether from what I can tell. Can you ensure that this fix produces the expected result given in the bug, or otherwise address why it's inappropriate to address the expected result? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misremembered. You'll still get "unexpected error", but you'll be notified that you supplied an invalid name. The buildplanner provides this error message. I've updated the PR description with an example to hopefully make this more clear. I hope we're not talking past each other. Sorry if I'm missing something. |
It occurs to me that maybe we want to recognize this as an input error though. We may need to add additional plumbing to recognize certain types of buildplanner errors as input errors. Should I do that here? Or should we do this in a new ticket? Or not at all? |
Sample: