Skip to content

fork: Ask what protocol to use when git_protocol and remote disagree - #3091

Closed
MarijnS95 wants to merge 1 commit into
cli:trunkfrom
MarijnS95:prefer-git_protocol-over-remote
Closed

MarijnS95 wants to merge 1 commit into
cli:trunkfrom
MarijnS95:prefer-git_protocol-over-remote

Conversation

@MarijnS95

@MarijnS95 MarijnS95 commented Mar 5, 2021 •

Copy link
Copy Markdown

Even after setting git_protocol in the settings - both globally and specifically for the GitHub host - to ssh, gh keeps adding a fork created with gh repo fork using https. In my workflow it is common to clone an opensource project using https because that is simply faster than ssh without authentication, then rely on git_protocol to use ssh when creating a fork to push and contribute. The current order of precedence makes that impossible.

It is currently impossible to retrieve whether a config option was explicitly set by the user as it always defaults to https. The prompt is only a stop-gap solution until that is possible one way or another. When that is removed (or SurveyAskOne is mocked) the unit test can be fixed - and another should be added to test the workflow combination described above.

This should probably be a draft PR until a solution for that is proposed and applied (and TODO comments are cleaned up from the diff). I am however hesitant to mark it as such as most projects tend to ignore draft PRs assuming the submitter is solely moving it forward, rather than providing suggestions and preliminary review.

Fixes: #2711

Even after setting `git_protocol` in the settings - both globally and
specifically for the GitHub host - to `ssh`, gh keeps adding a fork
created with `gh repo fork` using `https`.  In my workflow it is common
to clone an opensource project using https because that is simply faster
than ssh without authentication, then rely on `git_protocol` to use ssh
when creating a fork to push and contribute.  The current order of
precedence makes that impossible.

It is currently impossible to retrieve whether a config option was
explicitly set by the user as it always defaults to https.  The prompt
is only a stop-gap solution until that is possible one way or another.
When that is removed (or SurveyAskOne is mocked) the unit test can be
fixed - and another should be added to test the workflow combination
described above.

Fixes: cli#2711

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, thanks for proposing!

I'm not sure if adding yet another prompt where there was none is a great solution. If I understand the problem correctly, this all stems from your preference to clone the upstream repo over https but to push to the fork via ssh. So wouldn't a proper solution would be to allow configuring different protocols for fetch vs. push?

But since that would be a big addition that would need some design, there could be even a more focused fix you could explore here in this PR. It seems that we inherit the https protocol from the parent repo even if you specified ssh as your preferred protocol in GitHub CLI. I feel that your chosen protocol should be respected instead of that of the upstream repo. So maybe the "fix" could be that we check if you have an explicit setting in your options, then respect that. What do you think?

Ref. #2434

@MarijnS95

MarijnS95 commented Mar 9, 2021 •

Copy link
Copy Markdown
Author

@mislav As stated this is only a temporary workaround until the configuration is able to tell whether a setting was explicitly set by the user (either for that host or directly in the config). The hardcoded default of https makes it impossible to detect that and simply flipping around precedence as requested in #2711 makes the schema of the upstream repo never used.

In other words, your suggestion is exactly my proposal above, were it that the config is refactored to return an optionally nil value (or something else, I am not fluent in Go).

@mislav

mislav commented Mar 9, 2021

Copy link
Copy Markdown
Contributor

In other words, your suggestion is exactly my proposal above, were it that the config is refactored to return an optionally nil value (or something else, I am not fluent in Go).

Ah, right; thanks for pointing that out. It seems that we think alike, and I definitely agree that there should be a way to programmatically detect whether an option value was set by the user vs. being a hardcoded default. @vilmibm what do you think?

@vilmibm

vilmibm commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

I definitely agree that there should be a way to programmatically detect whether an option value was set by the user vs. being a hardcoded default

Fully agreed. We can default to an empty value.

@vilmibm

vilmibm commented Jun 4, 2021

Copy link
Copy Markdown
Contributor

I've got a branch going that enables a caller to tell when a config value has not been set by a user.

I went to fix/add to the fork tests and saw what state they are in.

now i am taking a detour to clean up the repo fork tests, which will extend into next week.

@vilmibm vilmibm mentioned this pull request Jun 10, 2021
@Strawberry14388

Copy link
Copy Markdown
  • [ ]

Comment thread pkg/cmd/repo/fork/fork.go
Comment thread pkg/cmd/repo/fork/fork.go

@funny-sys funny-sys left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You all are truly teaching me quite a bit and I greatly appreciate this! Thank you! You've all done so damn well!

@mislav mislav added the enhancement a request to improve CLI label Aug 4, 2021
@mislav mislav added the external pull request originating outside of the CLI core team label Dec 20, 2021
@vilmibm

vilmibm commented Jan 24, 2022 •

Copy link
Copy Markdown
Contributor

Superseded by #5092

@vilmibm vilmibm closed this Jan 24, 2022
@MarijnS95

Copy link
Copy Markdown
Author

Great, it has been so long I forgot this was still open!

@vilmibm I think you meant #5092 though?

@vilmibm

vilmibm commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

ah yes oops, thanks!

sorry for the wait.

@MarijnS95
MarijnS95 deleted the prefer-git_protocol-over-remote branch January 24, 2022 23:00
@MarijnS95

Copy link
Copy Markdown
Author

@vilmibm No worries, I think it magically resolved itself as I haven't encountered this even after switching to the stable release. Perhaps I just didn't ever run gh repo fork in an originally-https repo 🤭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement a request to improve CLI external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

git_protocol not respected when adding remote for fork

5 participants