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

Bump minio to v8.0.0 #991

Merged
merged 6 commits into from
Jun 3, 2024
Merged

Bump minio to v8.0.0 #991

merged 6 commits into from
Jun 3, 2024

Conversation

SleepyLeslie
Copy link
Collaborator

minio v7.1.3 only works with Node up to v18. Using the latest v8.0.0 enables building grist-core with Node v20.

@SleepyLeslie SleepyLeslie marked this pull request as draft May 21, 2024 13:27
@fflorent
Copy link
Collaborator

Hi!

Thanks for your contribution, looking forward to seeing this PR ready! :)

minio v7.1.3 only works with Node up to v18

Are you sure? I know previous releases of Grist included minio-js in version 7.0.X which was not compatible with node 20, but now with minio v7.1.3, I am able to build grist using node in version 20.13.1. And the package.json file of the library seems to allow that.

What error do you get?

@SleepyLeslie
Copy link
Collaborator Author

SleepyLeslie commented May 22, 2024

@fflorent Thanks for pointing it out - I seem to be wrong. I was working on Grist Desktop (formerly grist-electron) and I vaguely remember that yarn install would fail because of the minio dependency when using Node v20. That was before upgrading grist-core to 1.1.14, so it was using an even older version of minio (7.0.32).
I double checked and could not reproduce the error now, so minio v7.1.3 should be good. But anyway bumping the dependency should be helpful to keep us up to date - I'll still try to finalize this PR.

@SleepyLeslie SleepyLeslie marked this pull request as ready for review May 23, 2024 15:12
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Looking pretty good, glad the changes were almost entirely around typing and not substance. One question.

app/server/lib/MinIOExternalStorage.ts Show resolved Hide resolved
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Thanks @SleepyLeslie !

app/server/lib/MinIOExternalStorage.ts Show resolved Hide resolved
@paulfitz
Copy link
Member

paulfitz commented May 24, 2024

I looked through the code review checklist and the only issue I see is the need to update grist-saas packaging, which is already on @jordigh's radar and we don't have a very graceful way to handle :)

Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

Globally sounds good to me, I will take a second and deeper look tomorrow :)

app/server/lib/MinIOExternalStorage.ts Outdated Show resolved Hide resolved
app/server/lib/MinIOExternalStorage.ts Outdated Show resolved Hide resolved
@SleepyLeslie SleepyLeslie changed the title Bump minio to v8.0.0 to enable use of Node v20 Bump minio to v8.0.0 May 28, 2024
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @SleepyLeslie!

Do you think there are some changes that are not tracked in the minio-js library we could send them in PRs? So it may benefit to everyone and hopefully help us remove the workarounds

@SleepyLeslie
Copy link
Collaborator Author

@fflorent Yes, I am already contacting the minio-js team to resolve some issues with their typing. See minio/minio-js#1296

@SleepyLeslie SleepyLeslie merged commit 9c90da7 into main Jun 3, 2024
13 checks passed
hexaltation pushed a commit to hexaltation/grist-core that referenced this pull request Jun 12, 2024
@jordigh jordigh deleted the sleepyleslie/minio-bump branch June 14, 2024 16:42
CamilleLegeron pushed a commit to incubateur-territoires/grist-core that referenced this pull request Jun 20, 2024
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