-
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
Initial usage of buildplanner for state init
.
#2844
Conversation
mitchell-as
commented
Oct 24, 2023
•
edited by github-actions
bot
Loading
edited by github-actions
bot
DX-2271 `state init` uses buildplanner and not the mono api for creating commits |
I'm not really sure what I'm doing here, so feel free to correct me or suggest what I can do better. |
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.
Honestly, this looks really good so far. The error handling will need to be updated to work with this new mutation. It's not obvious how to do that and the implicit nature of graphQL + Go unmarshalling doesn't make it easier. I've left some examples that hopefully help get you started. Let me know if I can help clear anything up.
I started implementing user-facing errors for buildplanner errors raised during project creation because I think we're supposed to move away from |
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.
Almost there! My comments are primarily based on the knowledge that I picked up from working on the Build Planner code previously, so I wouldn't expect you to have known about them.
Use a params struct instead of arguments list. Make minimal buildexpression creation function even more minimal. Use timestamp from Platform rather than from current time. Forward buildplanner error messages.
I had to do a bit of cleanup. Let me know if I missed anything. |
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.
Looks good! Just one nit.
return &ProjectCreatedError{pcErr.Type} | ||
default: | ||
return errs.New(fallbackMessage) | ||
if pcErr.Type != "" { |
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.
Can we add a comment here and reference the user-facing errors story? We should be handling the specific error types but I know that's out of scope currently.