Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix additional long ids #1113

Merged
merged 1 commit into from
May 16, 2024
Merged

Fix additional long ids #1113

merged 1 commit into from
May 16, 2024

Conversation

Sineaggi
Copy link
Contributor

@Sineaggi Sineaggi commented Apr 23, 2024

Found a few cases where deserialization was throwing errors due to ints being out of out-of-range.

Follow-up to #811

@jmini
Copy link
Collaborator

jmini commented May 2, 2024

The change int -> Long in the models is a good idea, but it is breaking on the 5.x stream. Would it be OK to do this change only on 6.x?

@gwidion
Copy link

gwidion commented May 2, 2024

The change int -> Long in the models is a good idea, but it is breaking on the 5.x stream. Would it be OK to do this change only on 6.x?

If by "breaking" you mean fixing something that had no chance of working, then yes. 🙂

@jmini
Copy link
Collaborator

jmini commented May 2, 2024

Using int for the id will work only in on-prem setup (at the beginning, until the system reach the limit of int and then start to overflow).

Probably switching to Long is not hurting readers of this value too much.

@Sineaggi
Copy link
Contributor Author

Sineaggi commented May 2, 2024

Tests seem to be failing for reasons not relate to this change?

@Sineaggi
Copy link
Contributor Author

Sineaggi commented May 6, 2024

The test TestAvatarUpload keeps 502ing

@jmini
Copy link
Collaborator

jmini commented May 16, 2024

I think the integration tests needs to be reworked… It is too flaky right now.

@jmini jmini merged commit 346a02a into gitlab4j:main May 16, 2024
1 check passed
@jmini
Copy link
Collaborator

jmini commented May 16, 2024

I finally got a green build and I merged your change to main and 6.x. Thank you 🎉 .

@Sineaggi Sineaggi deleted the more-long-fixes branch May 16, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants