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: default file/uploads location for Silverstripe #5241

Conversation

Firesphere
Copy link
Contributor

@Firesphere Firesphere commented Aug 5, 2023

The Issue

ddev import-files on Silverstripe does not have a default import location set.

How This PR Solves The Issue

Adds a default UploadDirs method to Silverstripe, so the default (public/assets) is properly detected.

Manual Testing Instructions

Run an import of assets.

Automated Testing Overview

Added a test which checks for each known getUploadDirs method, if the return matches a set of default expected values.

Related Issue Link(s)

Depends on

Release/Deployment Notes

@Firesphere Firesphere changed the title Default file/uploads location for Silverstripe fix: default file/uploads location for Silverstripe Aug 5, 2023
@github-actions github-actions bot added the bugfix label Aug 5, 2023
@github-actions
Copy link

github-actions bot commented Aug 6, 2023

@Firesphere
Copy link
Contributor Author

Firesphere commented Aug 7, 2023

Seems so far the Colima tests just crashed. No big deal though, it's a 3-liner.
I'll make it fully working, and hopefully also, while at it, a few tiny fixes?

@rfay I noticed that the ddev list command works but since "Silverstripe" is longer than the currently allocated 9 characters, it gets a break.
Do you want a separate PR for that (Changing the limit from 9 to 13), or are you okay with putting it all in one PR?

I don't mind either way, but multiple PRs for just Silverstripe project-type related minor fixes feels a tad overkill?

edit
Seems now that testInternetIsActive is failing. It doesn't look related, but just checking, if it is?

@rfay
Copy link
Member

rfay commented Aug 7, 2023

@rfay I noticed that the ddev list command works but since "Silverstripe" is longer than the currently allocated 9 characters, it gets a break.
Do you want a separate PR for that (Changing the limit from 9 to 13), or are you okay with putting it all in one PR?

Separate PR, but anything you can do to improve the formatting is welcome. It's a real balancing act trying to make it work for the most possible situations. I do actually hate to make that column wider because it will make it work worse for more people (going past 80 characters, etc.)

@rfay rfay force-pushed the 20230806_firesphere_silverstripe_file_importupload branch from 54ced90 to 81a1455 Compare August 22, 2023 02:46
@rfay
Copy link
Member

rfay commented Aug 22, 2023

Rebased and the requisite PR is now pulled, so please check it over and take a look at the silverstripe files tarball. Thanks!

@Firesphere
Copy link
Contributor Author

I'll try to make some time free for it! But no promises... Life is surely getting in the way at the moment :)

@Firesphere
Copy link
Contributor Author

Firesphere commented Aug 30, 2023

I've added a very basic test that checks if the upload dir is at least not empty.

I'm a tad short on time right now, but I'm trying to get the tests to match an actual expectation instead of only "not empty"

*edit
Mildly unsure why the no-bind-mounts=false test keeps failing... It succeeds locally

@rfay
Copy link
Member

rfay commented Aug 30, 2023

Please edit the OP to add a complete description of how to test import-files, thanks!

Note that colima on amd64 has been broken for the last couple of weeks, so don't worry about that.

Mildly unsure why the no-bind-mounts=false test keeps failing... It succeeds locally

Not sure which test you're referring to, a link would be good.

image

@Firesphere
Copy link
Contributor Author

It's this one, the Colima test indeed :) :
https://github.com/ddev/ddev/actions/runs/6019812935/job/16330172143?pr=5241

@Firesphere Firesphere force-pushed the 20230806_firesphere_silverstripe_file_importupload branch from c401b74 to 52a62a4 Compare September 1, 2023 23:59
@rfay
Copy link
Member

rfay commented Sep 5, 2023

When you're ready for review, change to "Ready for review", thanks!

@rfay rfay force-pushed the 20230806_firesphere_silverstripe_file_importupload branch from 52a62a4 to a2b2a87 Compare September 23, 2023 03:35
@rfay
Copy link
Member

rfay commented Sep 23, 2023

Rebased. I hope things have calmed down now @Firesphere and that the new life is great! Let me know if you want to go forward with this one.

@rfay
Copy link
Member

rfay commented Sep 26, 2023

I'll close this about Oct 1 if it doesn't get attention, thanks @Firesphere . After that it can be reopened when you are ready to return to it.

@Firesphere
Copy link
Contributor Author

Apologies, both work and personal life are very stressful and busy at the moment.

I'll get back to tidying this up probably not before mid October

@Firesphere Firesphere force-pushed the 20230806_firesphere_silverstripe_file_importupload branch from a2b2a87 to 7076e06 Compare September 27, 2023 04:53
@Firesphere
Copy link
Contributor Author

Update, got some spare time.

I think this is what I completely intended to achieve.

Set the upload dir, and test for the known project types that have an upload dir, that it is what is expected from default settings.

@Firesphere Firesphere marked this pull request as ready for review September 27, 2023 08:06
@Firesphere Firesphere requested a review from a team as a code owner September 27, 2023 08:06
@rfay
Copy link
Member

rfay commented Sep 27, 2023

Thanks. I know what it's like to have too much else to do. Wasn't trying to pressure you, just try to keep things moving so it doesn't get too messy around here.

@rfay
Copy link
Member

rfay commented Oct 3, 2023

Sorry, this got lost in the older PRs and now I'm less available next few weeks. Will try to get it reviewed.

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

@Firesphere

Tested it with:

mkdir my-silverstripe-app
cd my-silverstripe-app
ddev config --project-type=silverstripe --docroot=public --create-docroot
ddev composer create --prefer-dist --no-scripts silverstripe/installer -y

mkdir my-assets && touch my-assets/asset.txt
ddev import-files --source=my-assets
Error: failed to import files for my-silverstripe-app: unable to import to /home/stas/code/ddev/my-silverstripe-app/public/public/assets: parent directory does not exist

But it tries to use public/public/assets. I believe it is because --docroot=public

I used quickstart https://ddev.readthedocs.io/en/latest/users/quickstart/#silverstripe

@Firesphere
Copy link
Contributor Author

Hm, okay, I did not expect that :D . I'll have a look. Cheers!

@Firesphere
Copy link
Contributor Author

Firesphere commented Nov 1, 2023

It was a tad dumb of me, the upload dir is indeed relative to the public dir, and it was indeed creating public/public/assets constantly.

A "simple" matter of not including public in the upload dir should solve it.

@rfay
Copy link
Member

rfay commented Nov 1, 2023

Thanks for the followup!

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let me know when you're happy with it and I'll pull.

@rfay rfay requested a review from stasadev November 2, 2023 01:38
@Firesphere
Copy link
Contributor Author

As far as I'm concerned/aware, it's good to go. It has all the tests and solves the original problem mentioned in #5234

@rfay
Copy link
Member

rfay commented Nov 2, 2023

@stasadev can take a look tomorrow and in it goes!

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rfay rfay merged commit 64c9e46 into ddev:master Nov 2, 2023
18 checks passed
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

3 participants