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

optionally can define endpoint URL #103

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

schneidermr
Copy link
Contributor

@schneidermr schneidermr commented Nov 18, 2023

With my changes, the S3 module will be able to support other storage with S3 compatible API's

Fixes: #28

@schneidermr
Copy link
Contributor Author

I'll check and fix that linter error, but the unit test error looks a bit strange to me. It seems like it's failing to another adapter

@schneidermr
Copy link
Contributor Author

@stnguyen90 Please confirm my suspicions, but as I see these lint problems are not related to my changes

@stnguyen90
Copy link
Contributor

unit test error looks a bit strange to me. It seems like it's failing to another adapter

Yes, you can ignore that failing unit test

these lint problems are not related to my changes

The lint problems might be related as it mentions the S3.php file and you modified that file. You can easily fix the lint problems by running the formatter (composer format).

@stnguyen90
Copy link
Contributor

stnguyen90 commented Nov 28, 2023

@schneidermr, were you able to test this adapter with a couple of different providers to make sure it works? Can you provide some screenshots of successful tests?

@schneidermr
Copy link
Contributor Author

@schneidermr, were you able to test this adapter with a couple of different providers to make sure it works? Can you provide some screenshots of successful tests?

I've to find some free providers for the test because I'm using GCP, but until that here is the demo with Cloudflare R2.

r2.mp4

@eldadfux
Copy link
Member

PR looks good, but we need tests to pass before we can merge.

@schneidermr
Copy link
Contributor Author

schneidermr commented Jan 17, 2024

PR looks good, but we need tests to pass before we can merge.

The authorization header is malformed; a non-empty Access Key (AKID) must be provided in the credential.

$key = $_SERVER['S3_ACCESS_KEY'] ?? '';

@eldadfux it seems the problem is not in the code.

@pietrodicaprio
Copy link

pietrodicaprio commented Jan 17, 2024

Checking the latest open PRs: it looks like they are all failing the unit tests due to the empty variable.

Are the secrets properly set in the repo and readable by the Action executed on a PR from a non-member?
The latest passing is #105 but the committer is a Contributor.

The secrets are not readable from actions run on pull_request. It's required to set pull_request_target as trigger. Here an example of a repo I own: https://github.com/Vanilla-OS/ABRoot/blob/main/.github/workflows/go-pr.yml#L4C23-L4C23

GitHub doc reference: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflows-in-forked-repositories and https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

@lohanidamodar lohanidamodar changed the base branch from main to feat-generic-s3 January 22, 2024 01:30
@lohanidamodar lohanidamodar merged commit c9138e5 into utopia-php:feat-generic-s3 Jan 22, 2024
2 of 3 checks passed
@pietrodicaprio
Copy link

Nice workaround @lohanidamodar , I hope you don't plan to do this for every PR 😆

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.

🚀 Feature: Implement a generic S3 adapter
5 participants