-
Notifications
You must be signed in to change notification settings - Fork 6
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
(self-hosting|2): add minio for local file uploads #974
base: main
Are you sure you want to change the base?
Conversation
…r allow users to disable email
…nt:' vars using vars from 'env_file'
@@ -118,7 +114,7 @@ jobs: | |||
INTEGRATION_TEST_HOST: localhost | |||
|
|||
- name: Print container logs | |||
if: failure() | |||
if: ${{failure() || cancelled()}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the tests are taking really long to fail and you cancel them, it's nice to be able to see the container logs
@@ -21,7 +21,7 @@ export function FileUploadPreview({ files }: { files: FileUpload }) { | |||
The file is <strong>{file.fileSize}</strong> bytes in size. Its | |||
MIME type is <strong>{file.fileType}</strong>. | |||
</p> | |||
<Button variant="secondary"> | |||
<Button variant="secondary" asChild> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was for the test, makes clicking this thing more consistent
@@ -25,6 +25,7 @@ export const env = createEnv({ | |||
ASSETS_REGION: z.string(), | |||
ASSETS_UPLOAD_KEY: z.string(), | |||
ASSETS_UPLOAD_SECRET_KEY: z.string(), | |||
ASSETS_STORAGE_ENDPOINT: z.string().url().optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the env var that determines whether or not you are using AWS or not
const bucket = env.ASSETS_BUCKET_NAME; | ||
|
||
const client = new S3Client({ | ||
endpoint: env.ASSETS_STORAGE_ENDPOINT, | ||
region: region, | ||
credentials: { | ||
accessKeyId: key, | ||
secretAccessKey: secret, | ||
}, | ||
forcePathStyle: !!env.ASSETS_STORAGE_ENDPOINT, // Required for MinIO | ||
}); | ||
const command = new PutObjectCommand({ | ||
Bucket: bucket, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are all the changes that are necessary to make minIO
(or any S3 compatible storage solution) work with our existing setup! isn't that great?
// don't continue going after 3 tests have failed, that's too many | ||
maxFailures: process.env.CI ? 3 : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe controversial. since every single test was failing and retrying 3 times, i'd rather know after 3 tests fail that i'm cooked already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it beautiful.
this is just to have something to upload
entrypoint: > | ||
/bin/sh -c ' | ||
/usr/bin/mc config host add myminio http://minio:9000 "$${MINIO_ROOT_USER}" "$${MINIO_ROOT_PASSWORD}"; | ||
/usr/bin/mc mb --ignore-existing myminio/"$${ASSETS_BUCKET_NAME}"; | ||
/usr/bin/mc anonymous set download myminio/"$${ASSETS_BUCKET_NAME}"; | ||
/usr/bin/mc admin user add myminio "$${ASSETS_UPLOAD_KEY}" "$${ASSETS_UPLOAD_SECRET_KEY}"; | ||
/usr/bin/mc admin policy attach myminio readwrite --user "$${ASSETS_UPLOAD_KEY}"; | ||
exit 0; | ||
' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be better to just have this in a script.
basically what this does:
- create the bucket
- allow public downloads for said bucket
- add a new user
- allow said user to upload
this sadly can't be done by just configuring some env vars for minio itself, very annoying, you need to use their mc
client. I'm not sure this is the best way to go about doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this explanation as a comment to the yml file?
core: | ||
build: | ||
context: . | ||
args: | ||
- PACKAGE=core | ||
container_name: core | ||
env_file: ./.env.docker-compose | ||
environment: | ||
#s non secret | ||
- ASSETS_BUCKET_NAME=assets.byron.pubpub.org | ||
- MAILGUN_SMTP_HOST=smtp.mailgun.org | ||
- MAILGUN_SMTP_PORT=465 | ||
- MAILGUN_SMTP_USERNAME=omitted | ||
- OTEL_SERVICE_NAME=core.core | ||
- PGDATABASE=postgres | ||
- PGHOST=host.docker.internal | ||
- PGPORT=54322 | ||
- PGUSER=postgres | ||
- PGPASSWORD=postgres | ||
- PUBPUB_URL=http://localhost:8080 | ||
networks: | ||
- app-network | ||
ports: | ||
- "30000:3000" | ||
|
||
core-nginx: | ||
build: ./infrastructure/nginx | ||
container_name: core-nginx | ||
environment: | ||
- NGINX_LISTEN_PORT=8080 | ||
- NGINX_PREFIX=/ | ||
- NGINX_UPSTREAM_HOST=core | ||
- NGINX_UPSTREAM_PORT=3000 | ||
- OTEL_SERVICE_NAME=core.nginx | ||
depends_on: | ||
- core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i removed the core
and jobs
build services for .dev
. I never use them, I don't think any of you use them
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies for the format here, prettier
seems to go back and forth whether it cares that this file has spaces or tabs. I think it makes sense to just make it the same as everywhere else
const Url = | ||
/^(?:https?|wss?|ftp):\/\/(?:\S+(?::\S*)?@)?(?:(?!(?:10|127)(?:\.\d{1,3}){3})(?!(?:169\.254|192\.168)(?:\.\d{1,3}){2})(?!172\.(?:1[6-9]|2\d|3[0-1])(?:\.\d{1,3}){2})(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5])){2}(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))|(?:(?:[a-z0-9\u{00a1}-\u{ffff}]+-)*[a-z0-9\u{00a1}-\u{ffff}]+)(?:\.(?:[a-z0-9\u{00a1}-\u{ffff}]+-)*[a-z0-9\u{00a1}-\u{ffff}]+)*(?:\.(?:[a-z\u{00a1}-\u{ffff}]{2,})))(?::\d{2,5})?(?:\/[^\s]*)?$/iu; | ||
/^(?:https?|wss?|ftp):\/\/(?:\S+(?::\S*)?@)?(?:(?!(?:10|127)(?:\.\d{1,3}){3})(?!(?:169\.254|192\.168)(?:\.\d{1,3}){2})(?!172\.(?:1[6-9]|2\d|3[0-1])(?:\.\d{1,3}){2})(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5])){2}(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))|(?:(?:[a-z0-9\u{00a1}-\u{ffff}]+-)*[a-z0-9\u{00a1}-\u{ffff}]+)(?:\.(?:[a-z0-9\u{00a1}-\u{ffff}]+-)*[a-z0-9\u{00a1}-\u{ffff}]+)*(?:\.(?:[a-z\u{00a1}-\u{ffff}]{2,}))|localhost)(?::\d{2,5})?(?:\/[^\s]*)?$/iu; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very smol modification: allow localhost
as a valid host to pass the Url
scheme. This was necessary in order to allow local uploads, and also it's just a valid URL
uppy.getPlugin<AwsS3Multipart<Meta, AwsBody>>(pluginName)!.setOptions({ | ||
// TODO: maybe use more specific types for Meta and Body | ||
getUploadParameters: async (file) => { | ||
if (!file || !file.type) { | ||
throw new Error("Could not read file."); | ||
} | ||
const signedUrl = await props.upload(file.name); | ||
return { | ||
method: "PUT", | ||
url: signedUrl, | ||
headers: { | ||
"content-type": file.type, | ||
}, | ||
}; | ||
}, | ||
}); | ||
useEffect(() => { | ||
uppy.getPlugin<AwsS3Multipart<Meta, AwsBody>>(pluginName)!.setOptions({ | ||
// TODO: maybe use more specific types for Meta and Body | ||
getUploadParameters: async (file) => { | ||
if (!file || !file.type) { | ||
throw new Error("Could not read file."); | ||
} | ||
|
||
if (!file.name) { | ||
throw new Error("File name is required"); | ||
} | ||
|
||
const signedUrl = await props.upload(file.name); | ||
|
||
if (typeof signedUrl === "object" && "error" in signedUrl) { | ||
throw new Error(signedUrl.error); | ||
} | ||
|
||
return { | ||
method: "PUT", | ||
url: signedUrl, | ||
headers: { | ||
"content-type": file.type, | ||
}, | ||
}; | ||
}, | ||
}); | ||
}, [props.upload]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not really change the logic (adds two extra checks but that's it)
this should be wrapped in an effect
though
<HoverCardPrimitive.Content | ||
ref={ref} | ||
align={align} | ||
sideOffset={sideOffset} | ||
className={cn( | ||
"z-50 w-64 rounded-md border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", | ||
className | ||
)} | ||
{...props} | ||
/> | ||
<HoverCardPortal> | ||
<HoverCardPrimitive.Content | ||
ref={ref} | ||
align={align} | ||
sideOffset={sideOffset} | ||
className={cn( | ||
"z-50 w-64 rounded-md border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", | ||
className | ||
)} | ||
{...props} | ||
/> | ||
</HoverCardPortal> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slightly hard to see, but the addition is of the <HoverCardPortal>
. Previously the little card that pops up over uploaded files would get obscured by a lot of things, making it hard to click in tests. Now with the portal this is fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! The environment variable and docker compose changes all look good to me.
Issue(s) Resolved
Mostly resolves #954
High-level Explanation of PR
This PR adds
minIO
, a self-hosted file server with an S3 compatible API, to our docker stack.This will allow users to host their files on their own server, instead of forcing them to set up an S3 bucket.
It also makes local development and testing a bit nicer for us, as we don't need to use our actual S3 credentials.
Reorganization
I (to my own detriment) reorganized the way the env vars are set for the dev/test compose images a bit. Instead of setting them in the
docker-compose.yml
, I load them through the corresponding.env.compose.(test|dev)
.This makes it slightly easier to manage and override, but also makes it slightly less explicit and more easy to lose track of due to the overriding (i got rid of that as much as possible though).
Let me know if you have any thoughts on this regard. Env var organization is ... annoying.
Test Plan
pnpm dev:setup
.env.local
if you have anycroccroc
, create a submissionConvince yourself that this is a good test
platform/core/playwright/fileUpload.spec.ts
Lines 1 to 133 in c25e50a
Screenshots (if applicable)
Notes