Skip to content

Added Path Style Endpoint - #670

Closed
Frostist wants to merge 2 commits into
catalyst:MOODLE_404_STABLEfrom
Frostist:MOODLE_404_STABLE
Closed

Added Path Style Endpoint#670
Frostist wants to merge 2 commits into
catalyst:MOODLE_404_STABLEfrom
Frostist:MOODLE_404_STABLE

Conversation

@Frostist

Copy link
Copy Markdown
Contributor

Used for alternative software endpoints like MinIo.

I am wanting to add this setting to the stable branch. Which will add a tickbox to allow uses that use alternative S3 storage methods other than AWS to store there data.

With Min.io it can't use the AWS standard and uses the "path style endpoint" which the AWS S3 plugin integrates with.

Used for alternative software endpoints like MinIo
@Frostist

Copy link
Copy Markdown
Contributor Author

@danmarsden If you believe this isn't necessary I can close the commit?

@danmarsden

Copy link
Copy Markdown
Member

I tend to leave the S3 checks to our AU devs to review unless it's a no-brainer. We use the swift end-points in the NZ team so I haven't looked at it that close.. @Peterburnett is this something you can take a look at?

@Frostist

Copy link
Copy Markdown
Contributor Author

Personally I have to add the line of code to our Moodle Servers to add this path style endpoint so we can use Minio, because it doesn't work with the normal endpoints.

Just thought others might want to use it as well

@Tchekda Tchekda left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not only Min.io but most of the other s3-like systems

@Frostist

Copy link
Copy Markdown
Contributor Author

Can I close this merge? Has it been merged to the main?

@Frostist Frostist left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🙌🏻

@Logiar

Logiar commented Aug 13, 2025

Copy link
Copy Markdown

If this gets merged #483 can be closed as well.

@Frostist

Frostist commented Aug 14, 2025

Copy link
Copy Markdown
Contributor Author

Nice to see others want this as well 🙌🏻

@Frostist

Copy link
Copy Markdown
Contributor Author

Is this going to be merged soon?

@Frostist Frostist closed this by deleting the head repository Jan 10, 2026
@Frostist

Frostist commented Jan 10, 2026

Copy link
Copy Markdown
Contributor Author

Hi guys, I am creating a new pull request to add this option, and also use the new Moodle/lib/aws-sdk file which comes pre-installed now anyways in Moodle. @Tchekda

@IfDougelseSa

Copy link
Copy Markdown

Hey guys, this plugin is amazing. Great job! I'm having the exact same problem, though. I noticed this PR 483 was created back in 2022 and a lot of people seem to be facing the same issue. Is there any chance this could get merged soon? Thanks!"

@danmarsden

Copy link
Copy Markdown
Member

@IfDougelseSa you note that you have the same problem, but don't state if you have tested the supplied patch to see if it fixes the issue you have? - If you want to help get this merged, it's more useful for you to test the patch and then post a comment on the open PR #716 to state how you tested it, on what version, what S3 tool (min.io or otherwise) and that it does what you needed it to.

We don't use Min.io ourselves, so it's hard for us to test properly.

@Frostist

Copy link
Copy Markdown
Contributor Author

@danmarsden I have a new push todo to this repo for the patch, but with moodle including the AWS plugin, this plugin with the patch "just works"

What do you need from me to prove it works? I can do a video recording for you?

@danmarsden

Copy link
Copy Markdown
Member

@Frostist as the original dev behind the patch - I believe it works "for you" but it's usually preferred that we get different people testing a patch other than the person who originally wrote it - for Moodle core patches, the code is usually tested by the original dev, a peer reviewer, the integration reviewer and then the QA tester before it actually lands in core. We don't have that same requirement here, but simply relying on the original dev (even/ or especially if that dev is me!...) isn't great practice.

@Frostist

Copy link
Copy Markdown
Contributor Author

100% understand. If you test @IfDougelseSa then let us know

@Frostist

Copy link
Copy Markdown
Contributor Author

See the new pull request here @IfDougelseSa

#716

@IfDougelseSa

Copy link
Copy Markdown

Hey guys! I applied a patch in a private branch last year and it works perfectly! However, since the plugin gets updated, I don't want to keep patching it manually every time. I'll test @Frostist's PR #716 and share the screenshots/results here.

@gabrielcruzmachado

Copy link
Copy Markdown

I'll test @Frostist's #716 ,i have the same problem and i will share results

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.

6 participants