KEMBAR78
Update test to be compatible with latest Glamour v0.8.0 by ottok · Pull Request #10151 · cli/cli · GitHub
Skip to content

Conversation

@ottok
Copy link
Contributor

@ottok ottok commented Dec 28, 2024

Latest Glamour has slightly changed logic in line length / wrapping, resulting test failures due to string mismatch. Update tests and bump dependency to v0.8.0.

This was detected then building the GitHub cli pacakge gh in Debian started to fail with src:golang-github-charmbracelet-glamour 0.8.0-1.

@ottok ottok requested a review from a team as a code owner December 28, 2024 23:27
@ottok ottok requested a review from williammartin December 28, 2024 23:27
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Dec 28, 2024
@cliAutomation
Copy link
Collaborator

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

@ottok ottok force-pushed the fix-glamour-v0.8.0-compat branch from d5ef708 to 173507a Compare December 28, 2024 23:46
@jtmcg
Copy link
Contributor

jtmcg commented Jan 2, 2025

Hey @ottok, thanks for the submission. It's not entirely clear to me where these failures are happening, as none of the tests are failing locally for me. Could you open an issue to link to this and describe the repro steps? Ultimately, I'd like to understand what's going on so we can catch it before release in the future.

@ottok
Copy link
Contributor Author

ottok commented Jan 3, 2025

Hi!

The original error was reported in Debian in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1091585 and fixed in https://salsa.debian.org/go-team/packages/gh/-/commit/8faccf149d0836f8ff285808182c15cf02dd9f28 that was included in the latest upload the Debian unstable.

Here is an example of one of the failures for reference:

=== RUN   TestPRReview_interactive
>     review_test.go:283: 
>         	Error Trace:	/<<PKGBUILDDIR>>/_build/src/github.com/cli/cli/v2/pkg/cmd/pr/review/review_test.go:283
>         	Error:      	Not equal: 
>         	            	expected: "Got:\n\n  cool story                                                                  \n\n"
>         	            	actual  : "Got:\n\n  cool story                                                                      \n\n"
>         	            	
>         	            	Diff:
>         	            	--- Expected
>         	            	+++ Actual
>         	            	@@ -2,3 +2,3 @@
>         	            	 
>         	            	-  cool story                                                                  
>         	            	+  cool story                                                                      
>         	            	 
>         	Test:       	TestPRReview_interactive
> --- FAIL: TestPRReview_interactive (0.00s)

I already put in a sizeable amount of effort into fixing this and submit upstream. It feels that your request to file a bug report about this is mostly process work and not very productive, so I will pass on it. Take this or close it and re-implement it yourself in the future when you decide to upgrade Glamour.

@jtmcg
Copy link
Contributor

jtmcg commented Jan 3, 2025

I already put in a sizeable amount of effort into fixing this and submit upstream. It feels that your request to file a bug report about this is mostly process work and not very productive, so I will pass on it.

Hey @ottok, while I understand this feeling, there's a few reasons we would appreciate a bug report beyond "process work"

  1. Issues are more discoverable than PRs for the community. This helps prevent other folks from putting a sizeable amount of effort into opening duplicate issues or implementing PRs that others have already put a sizeable amount of effort into. If wasting your time is your primary concern for not opening an issue, consider that that choice may result in others wasting time instead.
  2. Issues are our preferred form for facilitating conversations and understanding about what problems our users may be facing and aligning on a proposed fix. I, for one, am very unfamiliar with Debian, how these bugs are reported/addressed, and why we may need to upgrade Glamour. I would find value in the discussion that would follow in the issue. If I have questions, it is likely others do too, and having that conversation on an issue helps avoid the need to ask those questions again in the future.
  3. Our issue templates are designed to reduce the amount of back and forth necessary for maintainers and contributors to understand what is going on

Ultimately, my goal is to understand what is going on, but you have seemed to reject that premise as process. We have a lot of contributors to our tool, and try to do our best by them, but sometimes that requires a little help from them, too. I hope this demonstrates why my ask isn't just unnecessary process work.

@ottok
Copy link
Contributor Author

ottok commented Jan 5, 2025

Ok, as you spent that much effort in asking me to do it, I filed now #10179.

I also rebased this on latest trunk, and amended commit message with "Closes: #10179" so when you merge this, the bug report will auto-close.

@williammartin
Copy link
Member

@ottok, it looks like I can't push to your branch to fix the merge conflicts. It should just be a matter of removing the go mod changes and go getting the new version of glamour, rather than fighting the conflicts themselves. Happy to merge when that is done, sorry for the busy work.

@ottok
Copy link
Contributor Author

ottok commented Jan 22, 2025 via email

@williammartin
Copy link
Member

I'm ready to merge.

Latest Glamour has slightly changed logic in line length / wrapping,
resulting test failures due to string mismatch. Update tests and bump
dependency to v0.8.0, and others to the bare minimal level as generated
by `go mod tidy`.

This was detected then building the GitHub cli package `gh` in Debian
started to fail with src:golang-github-charmbracelet-glamour 0.8.0-1.

Closes: cli#10179
@ottok ottok force-pushed the fix-glamour-v0.8.0-compat branch from 2f07e86 to b19e682 Compare January 23, 2025 04:13
@ottok
Copy link
Contributor Author

ottok commented Jan 23, 2025

I dropped the existing go.{mod,sum} modifications, added manually the update github.com/charmbracelet/glamour v0.7.0 -> v0.8.0 and then ran go mod tidy go get the indirect dependencies updated automatically.

@williammartin williammartin merged commit 8c64de7 into cli:trunk Jan 23, 2025
@williammartin
Copy link
Member

Thanks for doing the work on this.

@andyfeller
Copy link
Member

@jtmcg @williammartin : are there any concerns about cli/go-gh still using the older versions of lipgloss and glamour?

module github.com/cli/go-gh/v2

go 1.21

require (
	github.com/AlecAivazis/survey/v2 v2.3.7
	github.com/MakeNowJust/heredoc v1.0.0
	github.com/charmbracelet/glamour v0.7.0
	github.com/charmbracelet/lipgloss v0.10.1-0.20240413172830-d0be07ea6b9c

@williammartin
Copy link
Member

None that I can think of. Would be fine to update them as well though.

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Feb 4, 2025
This MR contains the following updates:

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

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.66.1`](https://github.com/cli/cli/releases/tag/v2.66.1): GitHub CLI 2.66.1

[Compare Source](cli/cli@v2.66.0...v2.66.1)

#### Hotfix: `gh pr view` fails with provided URL

This addresses a regression in `gh pr view` was reported in [#&#8203;10352](cli/cli#10352). This regression was due to a change in `v2.66.0` that no longer allowed `gh pr` subcommands to execute properly outside of a git repo.

#### What's Changed

-   Hotfix: `gh pr view` fails with provided URL by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10354

**Full Changelog**: cli/cli@v2.66.0...v2.66.1

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

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

#### `gh pr view` and `gh pr status` now respect common triangular workflow configurations

Previously, `gh pr view` and `gh pr status` would fail for pull request's (MR) open in triangular workflows. This was due to `gh` being unable to identify the MR's corresponding remote and branch refs on GitHub.

Now, `gh pr view` and `gh pr status` should successfully identify the MR's refs when the following common git configurations are used:

-   [`branch.<branchName>.pushremote`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-branchltnamegtpushRemote) is set
-   [`remote.pushDefault`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-remotepushDefault) is set

Branch specific configuration, the former, supersedes repo specific configuration, the latter.

Additionally, if the [`@{push}` revision syntax](https://git-scm.com/docs/gitrevisions#Documentation/gitrevisions.txt-emltbranchnamegtpushemegemmasterpushemempushem) for git resolves for a branch, `gh pr view` and `gh pr status` should work regardless of additional config settings.

For more information, see

-   cli/cli#9363
-   cli/cli#9364
-   cli/cli#9365
-   cli/cli#9374

#### `gh secret list`, `gh secret set`, and `gh secret delete` now require repository selection when multiple `git` remotes are present

Previously, `gh secret list`, `gh secret set`, and `gh secret delete` would determine which remote to target for interacting with GitHub Actions secrets.  Remotes marked as default using `gh repo set-default` or through other `gh` commands had higher priority when figuring out which repository to interact with.  This could have unexpected outcomes when using `gh secret` commands with forked repositories as the upstream repository would generally be selected.

Now, `gh secret` commands require users to disambiguate which repository should be the target if multiple remotes are present and the `-R, --repo` flag is not provided.

For more information, see cli/cli#4688

#### Extension update notices now notify once every 24 hours per extension and can be disabled

Previously, the GitHub CLI would notify users about newer versions every time an extension was executed.  This did not match GitHub CLI notices, which only notified users once every 24 hours and could be disabled through an environment variable.

Now, extension update notices will behave similar to GitHub CLI notices.  To disable extension update notices, set the `GH_NO_EXTENSION_UPDATE_NOTIFIER` environment variable.

For more information, see cli/cli#9925

#### What's Changed

##### ✨ Features

-   Draft for discussing testing around extension update checking behavior by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9985
-   Make extension update check non-blocking by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10239
-   Ensure extension update notices only notify once within 24 hours, provide ability to disable all extension update notices by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9934
-   feat: make the extension upgrade fancier by [@&#8203;nobe4](https://github.com/nobe4) in cli/cli#10194
-   fix: padded display by [@&#8203;nobe4](https://github.com/nobe4) in cli/cli#10216
-   Update `gh attestation` attestation bundle fetching logic by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10185
-   Require repo disambiguation for secret commands by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10209
-   show error message for rerun workflow older than a month ago by [@&#8203;iamrajhans](https://github.com/iamrajhans) in cli/cli#10227
-   Update `gh attestation verify` table output by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10104
-   Enable MSI building for Windows arm64 by [@&#8203;dennisameling](https://github.com/dennisameling) in cli/cli#10297
-   feat: Add support for creating autolink references by [@&#8203;hoffm](https://github.com/hoffm) in cli/cli#10180
-   Find MRs using `@{push}` by [@&#8203;Frederick888](https://github.com/Frederick888) in cli/cli#9208
-   feat: Add support for viewing autolink references by [@&#8203;hoffm](https://github.com/hoffm) in cli/cli#10324
-   Update `gh attestation` bundle fetching logic by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10339

##### 🐛 Fixes

-   gh gist delete: prompt for gist id by [@&#8203;danochoa](https://github.com/danochoa) in cli/cli#10154
-   Better handling for waiting for codespaces to become ready by [@&#8203;cmbrose](https://github.com/cmbrose) in cli/cli#10198
-   Fix: `gh gist view` and `gh gist edit` prompts with no TTY by [@&#8203;mateusmarquezini](https://github.com/mateusmarquezini) in cli/cli#10048
-   Remove naked return values from `ReadBranchConfig` and `prSelectorForCurrentBranch` by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10197
-   Add job to deployment workflow to validate the tag name for a given release by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10121
-   \[gh run list] Stop progress indicator on failure from `--workflow` flag by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10323
-   Update deployment.yml by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10340

##### 📚 Docs & Chores

-   Add affected version heading to bug report issue form by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10269
-   chore: fix some comments by [@&#8203;petercover](https://github.com/petercover) in cli/cli#10296
-   Update triage.md to reflect FR experiment outcome by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10196
-   Clear up --with-token fine grained PAT usage by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10186
-   Correct help documentation around template use in `gh issue create` by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10208
-   chore: fix some function names in comment by [@&#8203;zhuhaicity](https://github.com/zhuhaicity) in cli/cli#10225
-   Tiny typo fix by [@&#8203;robmorgan](https://github.com/robmorgan) in cli/cli#10265
-   add install instructions for Manjaro Linux by [@&#8203;AMS21](https://github.com/AMS21) in cli/cli#10236
-   Update test to be compatible with latest Glamour v0.8.0 by [@&#8203;ottok](https://github.com/ottok) in cli/cli#10151
-   Add more `gh attestation verify` integration tests by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10102

##### :dependabot: Dependencies

-   Bump github.com/mattn/go-colorable from 0.1.13 to 0.1.14 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10215
-   Bump github.com/sigstore/protobuf-specs from 0.3.2 to 0.3.3 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10214
-   Bump github.com/gabriel-vasile/mimetype from 1.4.7 to 1.4.8 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10184
-   Bump google.golang.org/protobuf from 1.36.2 to 1.36.3 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10250
-   Bump golangci-linter and address failures to prepare for Go 1.24 strictness by [@&#8203;mikelolasagasti](https://github.com/mikelolasagasti) in cli/cli#10279
-   Bump github.com/google/go-containerregistry from 0.20.2 to 0.20.3 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10257
-   Bump actions/attest-build-provenance from 2.1.0 to 2.2.0 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10300
-   Bump google.golang.org/protobuf from 1.36.3 to 1.36.4 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10306
-   Upgrade sigstore-go to v0.7.0: fixes [#&#8203;10114](cli/cli#10114) formatting issue by [@&#8203;codysoyland](https://github.com/codysoyland) in cli/cli#10309
-   Bump github.com/in-toto/attestation from 1.1.0 to 1.1.1 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10319

#### New Contributors

Big thank you to our many new *and* longtime contributors making this release happen!! ❤️ ✨

-   [@&#8203;zhuhaicity](https://github.com/zhuhaicity) made their first contribution in cli/cli#10225
-   [@&#8203;danochoa](https://github.com/danochoa) made their first contribution in cli/cli#10154
-   [@&#8203;robmorgan](https://github.com/robmorgan) made their first contribution in cli/cli#10265
-   [@&#8203;iamrajhans](https://github.com/iamrajhans) made their first contribution in cli/cli#10227
-   [@&#8203;AMS21](https://github.com/AMS21) made their first contribution in cli/cli#10236
-   [@&#8203;petercover](https://github.com/petercover) made their first contribution in cli/cli#10296
-   [@&#8203;ottok](https://github.com/ottok) made their first contribution in cli/cli#10151
-   [@&#8203;dennisameling](https://github.com/dennisameling) made their first contribution in cli/cli#10297
-   [@&#8203;iamazeem](https://github.com/iamazeem) made their first contribution in cli/cli#10323
-   [@&#8203;Frederick888](https://github.com/Frederick888) made their first contribution in cli/cli#9208

**Full Changelog**: cli/cli@v2.65.0...v2.66.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:eyJjcmVhdGVkSW5WZXIiOiIzOS4xNDMuMCIsInVwZGF0ZWRJblZlciI6IjM5LjE0Ni40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
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.

5 participants