fix: prevent panic on gitlab import (releases/issues) #11282
mahlzahn
Gusted
patdyn
Labels
No labels
arch
riscv64
backport/v1.19
backport/v1.20
backport/v1.21/forgejo
backport/v10.0/forgejo
backport/v11.0/forgejo
backport/v12.0/forgejo
backport/v13.0/forgejo
backport/v14.0/forgejo
backport/v7.0/forgejo
backport/v8.0/forgejo
backport/v9.0/forgejo
breaking
bug
bug
confirmed
bug
duplicate
bug
needs-more-info
bug
new-report
bug
reported-upstream
code/actions
code/api
code/auth
code/auth/faidp
code/auth/farp
code/email
code/federation
code/git
code/migrations
code/packages
code/wiki
database
MySQL
database
PostgreSQL
database
SQLite
dependency-upgrade
dependency
certmagic
dependency
chart.js
dependency
Chi
dependency
Chroma
dependency
citation.js
dependency
codespell
dependency
css-loader
dependency
devcontainers
dependency
dropzone
dependency
editorconfig-checker
dependency
elasticsearch
dependency
enmime
dependency
F3
dependency
ForgeFed
dependency
garage
dependency
Git
dependency
git-backporting
dependency
Gitea
dependency
gitignore
dependency
go-ap
dependency
go-enry
dependency
go-gitlab
dependency
Go-org
dependency
go-rpmutils
dependency
go-sql-driver mysql
dependency
go-swagger
dependency
go-version
dependency
go-webauthn
dependency
gocron
dependency
Golang
dependency
goldmark
dependency
goquery
dependency
Goth
dependency
grpc-go
dependency
happy-dom
dependency
Helm
dependency
image-spec
dependency
jsonschema
dependency
KaTeX
dependency
lint
dependency
MariaDB
dependency
Mermaid
dependency
minio-go
dependency
misspell
dependency
Monaco
dependency
PDFobject
dependency
playwright
dependency
postcss
dependency
postcss-plugins
dependency
pprof
dependency
prometheus client_golang
dependency
protobuf
dependency
relative-time-element
dependency
renovate
dependency
reply
dependency
ssh
dependency
swagger-ui
dependency
tailwind
dependency
temporal-polyfill
dependency
terminal-to-html
dependency
tests-only
dependency
text-expander-element
dependency
urfave
dependency
vfsgen
dependency
vite
dependency
Woodpecker CI
dependency
x tools
dependency
XORM
Discussion
duplicate
enhancement/feature
forgejo/accessibility
forgejo/branding
forgejo/ci
forgejo/commit-graph
forgejo/documentation
forgejo/furnace cleanup
forgejo/i18n
forgejo/interop
forgejo/moderation
forgejo/privacy
forgejo/release
forgejo/scaling
forgejo/security
forgejo/ui
Gain
High
Gain
Nice to have
Gain
Undefined
Gain
Very High
good first issue
i18n/backport-stable
impact
large
impact
medium
impact
small
impact
unknown
Incompatible license
issue
closed
issue
do-not-exist-yet
issue
open
manual test
Manually tested during feature freeze
OS
FreeBSD
OS
Linux
OS
macOS
OS
Windows
problem
QA
regression
release blocker
Release Cycle
Feature Freeze
release-blocker
v7.0
release-blocker
v7.0.1
release-blocker
v7.0.2
release-blocker
v7.0.3
release-blocker
v7.0.4
release-blocker
v8.0.0
release-blocker/v9.0.0
run-all-playwright-tests
run-end-to-end-tests
test
manual
test
needed
test
needs-help
test
not-needed
test
present
untested
User research - time-tracker
valuable code
worth a release-note
User research - Accessibility
User research - Blocked
User research - Community
User research - Config (instance)
User research - Errors
User research - Filters
User research - Future backlog
User research - Git workflow
User research - Labels
User research - Moderation
User research - Needs input
User research - Notifications/Dashboard
User research - Rendering
User research - Repo creation
User research - Repo units
User research - Security
User research - Settings (in-app)
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/forgejo!11282
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "oliverpool/forgejo:gitlab-tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
It is unfortunately all mixed up, because refreshing the data, means breaking the tests. And changing the code means needing fresh data...
I advise looking at the commits in order:
(above changes the response testdata dumps in some way)
(below fixes some bugs and some tests, but without impacting the dump)
Release notes
The decision if the pull request will be shown in the release notes is up to the mergers / release team.
The content of the
release-notes/<pull request number>.mdfile will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.Release notes
632e97f94b6b2e7d46f06b2e7d46f0df5c202beeWIP: tests: refresh gitlab testdatato fix: prevent panic on gitlab import (releases/issues)df5c202beed486d87c95Thank you! Overall looks good and I have just some comments / observations.
@ -70,2 +70,3 @@}r.maxIssueIID = max(r.maxIssueIID, int64(issueIID))if r.frozen {panic("cannot record bigger issue IID after pull request IID generation has started")Is there any known case where this panic could happen?
It should only happen if the first issue is not the one with the largest IID.
It shouldn't happen (except if the
GetIssuescalls do not start with page 1 for instance)@ -818,3 +803,1 @@}return body}var mrFinder = regexp.MustCompile(`![0-9]+`)This regular expression might be a bit matching too much, e.g., here the regex for issue reference matching in markdown rendering:
issueNumericPattern = regexp.MustCompile(`(?:\s|^|\(|\[|\'|\")([#!][0-9]+)(?:\s|$|\)|\]|\'|\"|[:;,.?!]\s|[:;,.?!]$)`)You may have also references like
otherproject!123linking to an MR in another project. See https://docs.gitlab.com/user/project/issues/crosslinking_issues/.But, as mentioned below, there may be references to this project (for the test repo
test_repo!4) which should be replaced, too.You are right, but I think that this should be discussed in a dedicated issue first.
Here I tried to stick to the original logic (which was broken), that considered anything after a
!.I just created #11332 for that feature request.
@ -123,6 +123,7 @@ func TestGitlabDownloadRepo(t *testing.T) {releases, err := downloader.GetReleases()require.NoError(t, err)releases[0].Assets = nil // TODO: fix release testIn my opinion, these TODOs (I inserted them with my commit) can be solved in a follow-up. At least, now it shouldn’t panic any more and release assets were broken before already.
Add a test here for some references that should not be modified, e.g.,
projectX!5, actually more complicated, because iftest_repo!4should be modified 🤔As mentioned, I think this should be discussed in a dedicated issue (do we also want to replace full-links to the repo?)
@ -28,3 +25,3 @@[{"id":36554072,"name":"bug","description":null,"description_html":"","text_color":"#FFFFFF","color":"#d9534f","subscribed":false,"priority":null,"is_project_label":true},{"id":36554074,"name":"confirmed","description":null,"description_html":"","text_color":"#FFFFFF","color":"#d9534f","subscribed":false,"priority":null,"is_project_label":true},{"id":36554073,"name":"critical","description":null,"description_html":"","text_color":"#FFFFFF","color":"#d9534f","subscribed":false,"priority":null,"is_project_label":true},{"id":36554077,"name":"discussion","description":null,"description_html":"","text_color":"#FFFFFF","color":"#428bca","subscribed":false,"priority":null,"is_project_label":true},{"id":36554075,"name":"documentation","description":null,"description_html":"","text_color":"#1F1E24","color":"#f0ad4e","subscribed":false,"priority":null,"is_project_label":true},{"id":36556606,"name":"duplicate","description":"","description_html":"","text_color":"#FFFFFF","color":"#7F8C8D","subscribed":false,"priority":null,"is_project_label":true},{"id":36554079,"name":"enhancement","description":null,"description_html":"","text_color":"#FFFFFF","color":"#5cb85c","subscribed":false,"priority":null,"is_project_label":true},{"id":36554078,"name":"suggestion","description":null,"description_html":"","text_color":"#FFFFFF","color":"#428bca","subscribed":false,"priority":null,"is_project_label":true},{"id":36554076,"name":"support","description":null,"description_html":"","text_color":"#1F1E24","color":"#f0ad4e","subscribed":false,"priority":null,"is_project_label":true},{"id":36554080,"name":"test-scope::label0","description":"scoped label","description_html":"scoped label","text_color":"#FFFFFF","color":"#6699cc","subscribed":false,"priority":null,"is_project_label":true},{"id":36554094,"name":"test-scope::label1","description":"","description_html":"","text_color":"#FFFFFF","color":"#dc143c","subscribed":false,"priority":null,"is_project_label":true}][{"id":36554072,"name":"bug","description":null,"description_html":"","text_color":"#FFFFFF","color":"#d9534f","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554074,"name":"confirmed","description":null,"description_html":"","text_color":"#FFFFFF","color":"#d9534f","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554073,"name":"critical","description":null,"description_html":"","text_color":"#FFFFFF","color":"#d9534f","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554077,"name":"discussion","description":null,"description_html":"","text_color":"#FFFFFF","color":"#428bca","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554075,"name":"documentation","description":null,"description_html":"","text_color":"#1F1E24","color":"#f0ad4e","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36556606,"name":"duplicate","description":"","description_html":"","text_color":"#FFFFFF","color":"#7F8C8D","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554079,"name":"enhancement","description":null,"description_html":"","text_color":"#FFFFFF","color":"#5cb85c","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554078,"name":"suggestion","description":null,"description_html":"","text_color":"#FFFFFF","color":"#428bca","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554076,"name":"support","description":null,"description_html":"","text_color":"#1F1E24","color":"#f0ad4e","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554080,"name":"test-scope::label0","description":"scoped label","description_html":"scoped label","text_color":"#FFFFFF","color":"#6699cc","archived":false,"subscribed":false,"priority":null,"is_project_label":true},{"id":36554094,"name":"test-scope::label1","description":"","description_html":"","text_color":"#FFFFFF","color":"#dc143c","archived":false,"subscribed":false,"priority":null,"is_project_label":true}]Means, we can now support the migration of the state of archived labels, too. -> feature request (not necessary with this PR)
Created #11331 for that now (although finally, I figured out that gitlab.com currently does not support label archiving, yet.)
@ -1,17 +0,0 @@Referrer-Policy: strict-origin-when-cross-originThis file (and other
…gitea…files) got removed because we don’t test on it any more?yes, I believe they were never cleaned up (the added
os.MkdirAllmakes it easier to delete such files)@ -0,0 +1,29 @@Accept-Ranges: bytesThat move makes sense, thanks.
@ -299,3 +371,3 @@PosterName: "patdyn",Created: time.Date(2025, time.November, 25, 9, 49, 0, 750000000, time.UTC),Content: "Although we had some trouble with !4",Content: "Although we had some trouble with !5.",Good fix, too. The dot was missing.
This was fixed by using the regex 👼
@mahlzahn do you mind creating a new issue to propose a better handling of MR references?
Currently any
![0-9]+gets replaced, but this is:other!4test_repo!4n!4g!3issueNumericPattern = regexp.MustCompile(`(?:\s|^|\(|\[|\'|\")([#!][0-9]+)(?:\s|$|\)|\]|\'|\"|[:;,.?!]\s|[:;,.?!]$)`)Furthermore, full links inside the repo could be replaced as well.
I think that such changes would be more of a feature (I tried to limit this PR to bugfixing), hence the suggestion of a dedicated issue.
@oliverpool wrote in #11282 (comment):
I'm fine with that! This PR is fixing the panics and the rest will be features/cosmetic bug fixes.
Edit: I just created #11332 for that feature request.
8ff3ec103143ed286e49Where does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/11282.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.This message and the release notes originate from a call to the release-notes-assistant.
Release notes
Where does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/11282.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.This message and the release notes originate from a call to the release-notes-assistant.
Release notes
@ -71,2 +73,2 @@for headerName, headerValues := range response.Header {for _, headerValue := range headerValues {for _, headerName := range slices.Sorted(maps.Keys(response.Header)) {for _, headerValue := range response.Header[headerName] {Why is this changed?
To reduce the diff when refreshing the testdata.
For instance
8ff3ec1031is more readable than !11282 (commitc5f9421aa6)Ah, thanks.
I think that the PR title (or a release notes file) could be a bit more verbose to explain what kind of migrations were fixed:
Where does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/11282.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.This message and the release notes originate from a call to the release-notes-assistant.
Release notes
e312986e7170ae7410abWhere does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/11282.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.This message and the release notes originate from a call to the release-notes-assistant.
Release notes
@ -123,6 +123,9 @@ func TestGitlabDownloadRepo(t *testing.T) {releases, err := downloader.GetReleases()require.NoError(t, err)// TODO: fix size, currently reported as 0I think having these TODOs fixed would be good before merge.
Otherwise it might be confusing to other devs working with these tests.
LGTM otherwise, built your branch and migrated the linked repo without problems.
I plan to work on it after the merge of this PR (in near future), but at least with this PR, the import won't panic anymore. It needs some design decisions before fixing the release assets import.
Okay, that makes sense. Could you then leave a reference with these TODOs for others who might stumble across? Edit: Reference as in a Link to the Issue or this PR so people will know this is being worked on.
OK, done: !11282 (commit
e98d13fbc7)e98d13fbc76b5edf8408… and rebased:
e98d13fbc7..6b5edf8408