-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Support triangular git workflows in pr create
#238
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
Conversation
- The local git remotes are scanned and resolved to GitHub repositories - The "base" repo is the first result resolved to its parent repo (if a fork) - The name of the default branch is read from the base repo - The "head" repo is the first repo that has push access
vilmibm
left a comment
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'd love to see more code documentation around the new graphql queries since they are complex and currently (due to the nature of working with graphql) kind of opaque.
the code itself i'm 👍 on and am pleased to see this complicated bit of github interaction being tackled with clarity and structure.
api/queries_repo.go
Outdated
| `, i, repo.RepoOwner(), repo.RepoName())) | ||
| } | ||
|
|
||
| type ViewerOrRepo struct { |
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.
unfortunately i think this might need some code commenting to clarify. i've read the function a few times but without also studying the graphql API i'm having a hard time figuring out what is going on here.
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.
Good catch— this struct is a remnant of an older, messier implementation that I forgot to clean up! Fixed
vilmibm
left a comment
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.
Code is much easier to follow with the comments, thanks!
I'm 👍 on all this but wonder if the remote-handling stuff is worthy of unit tests?
This leads to unwanted consequences in `pr create`
|
@vilmibm Absolutely; good call. I've intended to add unit tests before merging, since current |
|
cool, I'll leave it for you to merge then once tests are in. |
Support triangular git workflows in `pr create`
) Related to cli#238 Co-authored-by: Mackenzie Zastrow <zastrowm@users.noreply.github.com>
We've received customer confusion regarding some of the errors when attempting to run on bedrock. To help customers understand what's going wrong, add region and model information to exceptions coming out of the bedrock provider & point them to our docs when possible. Semi-related to cli#238 Co-authored-by: Mackenzie Zastrow <zastrowm@users.noreply.github.com>
Only use a default region if the session doesn't provide one or if the AWS_REGION environment variable is not set. Fixes cli#238
--repoflag when selecting base repoTODO:
Respect existing upstream configuration of the current branch (if already pushed)(follow-up)Fixes #172, fixes #174, fixes #224