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 S3 integration custom endpoints usage #15701

Merged
merged 2 commits into from
Mar 10, 2025
Merged

Conversation

CSLTech
Copy link
Contributor

@CSLTech CSLTech commented Mar 8, 2025

Description

Custom S3 endpoints are broken. This fixes the S3 integration usage of custom S3 endpoints

Addresses

Launchcontrol

Fix S3 integration when using custom endpoints

@CSLTech CSLTech requested a review from a team as a code owner March 8, 2025 15:32
@CSLTech CSLTech requested review from samwho and removed request for a team March 8, 2025 15:32
Copy link

qa-wolf bot commented Mar 8, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link
Contributor

github-actions bot commented Mar 8, 2025

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@samwho samwho enabled auto-merge March 10, 2025 11:51
@samwho samwho disabled auto-merge March 10, 2025 11:51
@samwho
Copy link
Collaborator

samwho commented Mar 10, 2025

Hey @CSLTech, thanks for the contribution!

I'll get this merged when the tests pass.

@samwho
Copy link
Collaborator

samwho commented Mar 10, 2025

Actually, @mike12345567 pointed out to me that folks may have set this to invalid values and forgotten they've done it since it does nothing right now. Enabling this could break them.

What we'd like to do is introduce a new config parameter called customEndpoint, and deprecate this endpoint parameter. Would you be able to make this change?

What you need to do is add deprecated: true to the config entry in the SCHEMA on line 45. Then add a new customEndpoint to the schema, as well as the to S3Config type above the schema. Then plumb it in like you did with config.endpoint 🙏

@CSLTech
Copy link
Contributor Author

CSLTech commented Mar 10, 2025

Ermm, I can make the change, but this should be somewhat documented as this PR in it's current state fixes an issue that is a regression from a working behavior. Moreover if they had it misconfigured it would have made the signed links behave in a (Most likely broken) weird way.

@samwho
Copy link
Collaborator

samwho commented Mar 10, 2025

@CSLTech Apologies, you're right. I misunderstood the situation. Thank you for noticing and fixing this!

@samwho samwho merged commit d0026f8 into Budibase:master Mar 10, 2025
22 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants