KEMBAR78
fix(repo fork): add non-TTY output when fork is newly created by aryanbhosale · Pull Request #10158 · cli/cli · GitHub
Skip to content

Conversation

@aryanbhosale
Copy link
Contributor

Fixes #10079

Previously, when running gh repo fork in a non-interactive environment (non-TTY) for a brand-new fork, there was no output—causing confusion for CI/CD scripts. This change adds an else block to ensure we print "Created fork " even in non-TTY mode, matching the behavior when a fork already exists.

@aryanbhosale aryanbhosale requested a review from a team as a code owner January 1, 2025 06:28
@aryanbhosale aryanbhosale requested a review from jtmcg January 1, 2025 06:28
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Jan 1, 2025
Copy link
Contributor

@jtmcg jtmcg left a comment

Choose a reason for hiding this comment

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

Hey @aryanbhosale, thanks for getting started on this. This does look like it's headed in the right direction, but there are three things that need to be addressed:

  1. The Acceptance Criteria on the issue says we should display this on stdout while you've got it printing to stderr.
  2. The Acceptance Criteria on the issue also says that we should be returning the url, not the message you have here.
  3. There are a handful of failing tests that need to be fixed.

Let me know if you've got any questions on how to address these - I'm happy to help in any way I can 🙂

@aryanbhosale
Copy link
Contributor Author

Hey @aryanbhosale, thanks for getting started on this. This does look like it's headed in the right direction, but there are three things that need to be addressed:

  1. The Acceptance Criteria on the issue says we should display this on stdout while you've got it printing to stderr.
  2. The Acceptance Criteria on the issue also says that we should be returning the url, not the message you have here.
  3. There are a handful of failing tests that need to be fixed.

Let me know if you've got any questions on how to address these - I'm happy to help in any way I can 🙂

Hey @jtmcg ,
Thanks for the feedback, I've made the change in the file but it's still failing some tests. Could you help me find why that's happening? Earlier it was understood that due to it being printed on stderr instead of stdout it threw errors, but now it is on stdout yet these errors.

@jtmcg
Copy link
Contributor

jtmcg commented Jan 4, 2025

Hey @jtmcg ,
Thanks for the feedback, I've made the change in the file but it's still failing some tests. Could you help me find why that's happening? Earlier it was understood that due to it being printed on stderr instead of stdout it threw errors, but now it is on stdout yet these errors.

Hey @aryanbhosale, I'm happy to help. I've opened this PR against your branch with the test fixes and tried my best to explain how I found what needed to be fixed in the PR description. Let me know if you have more questions about it 🙂

@aryanbhosale
Copy link
Contributor Author

Hey @jtmcg ,
Thanks for the feedback, I've made the change in the file but it's still failing some tests. Could you help me find why that's happening? Earlier it was understood that due to it being printed on stderr instead of stdout it threw errors, but now it is on stdout yet these errors.

Hey @aryanbhosale, I'm happy to help. I've opened this PR against your branch with the test fixes and tried my best to explain how I found what needed to be fixed in the PR description. Let me know if you have more questions about it 🙂

This was really helpful! Thanks a lot for this:)

@aryanbhosale aryanbhosale requested a review from jtmcg January 4, 2025 16:00
Copy link
Contributor

@jtmcg jtmcg left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for doing this 🙌

@jtmcg jtmcg merged commit 1f4e005 into cli:trunk Jan 6, 2025
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Jan 8, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [cli/cli](https://github.com/cli/cli) | minor | `v2.64.0` -> `v2.65.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>cli/cli (cli/cli)</summary>

### [`v2.65.0`](https://github.com/cli/cli/releases/tag/v2.65.0): GitHub CLI 2.65.0

[Compare Source](cli/cli@v2.64.0...v2.65.0)

#### What's Changed

-   Document the base repo resolution functions by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10110
-   Update releasing.md by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10116
-   Document how to set gh-merge-base by [@&#8203;heaths](https://github.com/heaths) in cli/cli#10112
-   Upgrade golang.org/x/net to v0.33.0 by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10135
-   add pending status for workflow runs by [@&#8203;dziamidchyk](https://github.com/dziamidchyk) in cli/cli#10143
-   Remove release discussion posts and clean up related block in deployment yml by [@&#8203;shauryatiwari1](https://github.com/shauryatiwari1) in cli/cli#10145
-   docs(repo): make explicit which branch is used when creating a repo by [@&#8203;nobe4](https://github.com/nobe4) in cli/cli#10163
-   feat: Add support for listing autolink references by [@&#8203;hoffm](https://github.com/hoffm) in cli/cli#10124
-   Add mention of classic token in gh auth login docs by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10164
-   Feat: Allow setting security_and_analysis settings in gh repo edit by [@&#8203;ChandranshuRao14](https://github.com/ChandranshuRao14) in cli/cli#10139
-   Upgrade generated workflows by [@&#8203;jsoref](https://github.com/jsoref) in cli/cli#10181
-   Myriad fixes to provide clarity on determining tracking ref in MR create by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10187
-   Handle missing upstream configs for `gh pr create`  by [@&#8203;cmbrose](https://github.com/cmbrose) in cli/cli#10177
-   fix(repo fork): add non-TTY output when fork is newly created by [@&#8203;aryanbhosale](https://github.com/aryanbhosale) in cli/cli#10158
-   Bump cli/go-gh for indirect security vulnerability by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10190

#### New Contributors

-   [@&#8203;dziamidchyk](https://github.com/dziamidchyk) made their first contribution in cli/cli#10143
-   [@&#8203;shauryatiwari1](https://github.com/shauryatiwari1) made their first contribution in cli/cli#10145
-   [@&#8203;hoffm](https://github.com/hoffm) made their first contribution in cli/cli#10124
-   [@&#8203;ChandranshuRao14](https://github.com/ChandranshuRao14) made their first contribution in cli/cli#10139

**Full Changelog**: cli/cli@v2.64.0...v2.65.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS45MS4yIiwidXBkYXRlZEluVmVyIjoiMzkuOTEuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When gh repo fork results in a newly created fork, there is no output to non-TTY terminal

3 participants