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

S3: add config flag to proxy S3 media #1014

Merged
merged 3 commits into from
Nov 11, 2022

Conversation

LittleFox94
Copy link
Contributor

This adds a config value to proxy S3 media through GtS instead of redirecting, while still defaulting to redirect.

@tsmethurst
Copy link
Contributor

Heya, to get this passing CI tests you gotta update the test scripts test/cliparsing.sh and test'/envparsing.sh.

Also, what will the behavior be like if a null url is returned? Will it then stream directly to the caller from s3 via gts?

@LittleFox94
Copy link
Contributor Author

Heya, to get this passing CI tests you gotta update the test scripts test/cliparsing.sh and test'/envparsing.sh.

Ah, oki - will do! I suppose test/cliparsing.sh isn't required to be changed as there are virtually no CLI arguments for S3 config? Thinking of creating a PR to add them as well

Also, what will the behavior be like if a null url is returned? Will it then stream directly to the caller from s3 via gts?

I did not touch the existing behavior for null URLs, but tested it is actually proxying the data from S3 through GtS to the client. I suppose it is streaming the data, but didn't verify that - probably would be fit for another issue or PR anyway though.

@tsmethurst
Copy link
Contributor

there are virtually no CLI arguments for S3 config

Oh are they missing? I think they need to be added here: https://github.com/superseriousbusiness/gotosocial/blob/main/internal/config/flags.go#L87

@LittleFox94
Copy link
Contributor Author

there are virtually no CLI arguments for S3 config

Oh are they missing? I think they need to be added here: https://github.com/superseriousbusiness/gotosocial/blob/main/internal/config/flags.go#L87

yep, saw that when I wanted to add my new flag ^^

I also still have that local branch to generate the defaults and flags from the config struct ... maybe polishing that would be great x)

@tsmethurst
Copy link
Contributor

local branch to generate the defaults and flags from the config struct

We have this generatey stuff already, check it out! https://github.com/superseriousbusiness/gotosocial/blob/main/internal/config/config.go#L45

@LittleFox94
Copy link
Contributor Author

local branch to generate the defaults and flags from the config struct

We have this generatey stuff already, check it out! https://github.com/superseriousbusiness/gotosocial/blob/main/internal/config/config.go#L45

isn't this only generating the config access helpers? Not the command line flags and defaults?

And yeah, saw that and had to run it for my new config option - and am thinking of adding a PR to have go generate it automatically on building via //go:generate stuff

@LittleFox94
Copy link
Contributor Author

Added the new option to test/envparsing.sh, CI should be ok now

I think adding the command line flags is better for another PR

@LittleFox94
Copy link
Contributor Author

OK, test/cliparsing.sh needed to be changed 🙃

@tsmethurst
Copy link
Contributor

Thank you!

@tsmethurst tsmethurst merged commit 948e90b into superseriousbusiness:main Nov 11, 2022
@LittleFox94 LittleFox94 deleted the s3-proxy-mode branch November 11, 2022 12:21
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.

None yet

2 participants