Skip to content

Conversation

@mislav
Copy link
Contributor

@mislav mislav commented Jan 21, 2020

  • 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
  • Fixes respecting global --repo flag when selecting base repo

TODO:

  • Fix tests
  • Auto-fork repository when no suitable head repo
  • Respect existing upstream configuration of the current branch (if already pushed) (follow-up)

Fixes #172, fixes #174, fixes #224

- 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
@mislav mislav requested a review from vilmibm January 21, 2020 17:28
Copy link
Contributor

@vilmibm vilmibm left a 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.

`, i, repo.RepoOwner(), repo.RepoName()))
}

type ViewerOrRepo struct {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

@vilmibm vilmibm left a 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`
@mislav
Copy link
Contributor Author

mislav commented Jan 22, 2020

@vilmibm Absolutely; good call. I've intended to add unit tests before merging, since current pr create tests only exercise a few happy paths.

@vilmibm
Copy link
Contributor

vilmibm commented Jan 22, 2020

cool, I'll leave it for you to merge then once tests are in.

@mislav mislav merged commit 530bf84 into master Jan 23, 2020
@mislav mislav deleted the pr-create-just-works-TM branch January 23, 2020 16:08
mislav added a commit that referenced this pull request Jan 23, 2020
Support triangular git workflows in `pr create`
Stonre pushed a commit to Stonre/strands-fork that referenced this pull request Jul 21, 2025
)

Related to cli#238

Co-authored-by: Mackenzie Zastrow <zastrowm@users.noreply.github.com>
Stonre pushed a commit to Stonre/strands-fork that referenced this pull request Jul 21, 2025
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>
Stonre pushed a commit to Stonre/strands-fork that referenced this pull request Jul 21, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants