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

[3.x][Filesystem] Provide an additional argument for tenant name path #817

Merged
merged 3 commits into from
May 26, 2022

Conversation

vstyler96
Copy link
Contributor

@vstyler96 vstyler96 commented Mar 4, 2022

Summary of changes

During the work of an application that uses Google Cloud Storage as the main storage method. I found that if we want to save a file without overriding the default root path for GCS, the FilesystemTenancyBootstrapper adds an additional folder in the GCS Filesystem called /.

image

So I decided to pass a custom folder path.

Case 1: using %storage_path%

If we try to use the param %storage_path% this will add the full path to our project into the GCS filesystem.
image

Case 2: using a custom path without %storage_path%

I tried to pass a custom path, but this does not include the tenant folder. so the files from all tenants are mixed with each other (not expected)
image

So my resolution includes a minor update to the $finalPrefix variable. If we add a %tenant% variable, this will let us pass a more customizable path. So we can pass the /a/random/path/for/my/tenant/%tenant% path or any other path manually for each tenant in filesystems like GCS or S3

In this PR we let the user pass an additional parameter using `%tenant%` so the user can additionally pass the folder corresponding to each tenant.
This is my proposal, because if I try to use %storage_path% within Linux, I get the full path to the project when I use Google Cloud Storage
@vstyler96 vstyler96 changed the title Let the user pass the tenant suffix by %tenant% Let the user pass the tenant parameter by %tenant% Mar 4, 2022
@vstyler96 vstyler96 changed the title Let the user pass the tenant parameter by %tenant% [3.x] Let the user pass the tenant parameter by %tenant% Mar 4, 2022
Moving from $subject to $root when %storage_path% has been replaced
@stein-j
Copy link
Contributor

stein-j commented Mar 5, 2022

Isn't the tenant folder created automatically ? So you simply need to do:

'root_override' => [
            // ...
            'gcs' => 'tenants/',
],

Unless if I'm missing something

@vstyler96
Copy link
Contributor Author

vstyler96 commented Mar 5, 2022

Isn't the tenant folder created automatically ? So you simply need to do:

'root_override' => [
            // ...
            'gcs' => 'tenants/',
],

Unless if I'm missing something

I already made it and all of the files from all tenants are stored in tenants/
image

@stancl
Copy link
Member

stancl commented Mar 5, 2022

Yeah I think @stein-j's suggestion should work? Not exactly sure what the problem is here. S3 doesn't use the storage_path at all and it stores files like /tenant123/foo.txt

@vstyler96
Copy link
Contributor Author

Yeah I think @stein-j's suggestion should work? Not exactly sure what the problem is here. S3 doesn't use the storage_path at all and it stores files like /tenant123/foo.txt

Using GCS the path adds a "/" folder as root

@stancl
Copy link
Member

stancl commented Mar 6, 2022

Using GCS the path adds a "/" folder as root

What do you mean by that?

@vstyler96
Copy link
Contributor Author

Using GCS the path adds a "/" folder as root

What do you mean by that?

image
image

@stancl
Copy link
Member

stancl commented Mar 7, 2022

I still don't understand. If you want this feature added, please explain clearly what the issue is and how this would solve it.

@vstyler96 vstyler96 changed the title [3.x] Let the user pass the tenant parameter by %tenant% [3.x][Filesystem] Provide an additional argument for tenant path Mar 7, 2022
@vstyler96
Copy link
Contributor Author

@stancl I just updated the description :D. I tried to be clearer.

Also as mentioned in #802 (comment), I'll wait for that merge and update this PR after that.

@stancl
Copy link
Member

stancl commented Mar 30, 2022

Note that I just pushed some changes to this logic

@jncalderon
Copy link

@stancl @vstyler96 some any news?

@stancl
Copy link
Member

stancl commented May 13, 2022

@vstyler96 Could you resolve the conflicts?

@vstyler96
Copy link
Contributor Author

vstyler96 commented May 13, 2022

@stancl Ready to go! 🤝
image
image

P.S: Also removed a no longer used class on use declaration section

@vstyler96 vstyler96 changed the title [3.x][Filesystem] Provide an additional argument for tenant path [3.x][Filesystem] Provide an additional argument for tenant name path May 13, 2022
@stancl stancl added the ready label May 15, 2022
@jncalderon
Copy link

@stancl is ready :)

@stancl stancl merged commit 51228de into archtechx:3.x May 26, 2022
@stancl
Copy link
Member

stancl commented May 26, 2022

Thanks!

@vstyler96 vstyler96 deleted the patch-1 branch May 26, 2022 14:36
@danpalmieri
Copy link

i'm getting mkdir(): File exists error on [app / Jobs / CreateFrameworkDirectoriesForTenant.php : 21]
trying to create a new tenant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants